Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl Map::retain and Set::retain #27

Merged
merged 4 commits into from
Oct 14, 2022
Merged

impl Map::retain and Set::retain #27

merged 4 commits into from
Oct 14, 2022

Conversation

pitaj
Copy link
Collaborator

@pitaj pitaj commented Oct 14, 2022

also test examples and benches in CI

relates #11

test examples and benches in CI
Copy link
Owner

@udoprog udoprog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retain impl LGTM, I'd like the examples to be cleaner though. Easiest would be to only build examples with the full set of features (and no need to #[test] them).

examples/basic.rs Outdated Show resolved Hide resolved
examples/map_feature.rs Outdated Show resolved Hide resolved
@udoprog udoprog force-pushed the retain branch 2 times, most recently from bac123c to b0885ab Compare October 14, 2022 15:47
@pitaj
Copy link
Collaborator Author

pitaj commented Oct 14, 2022

Understood on the examples, but can you restore the one you deleted (map_feature) and just remove the feature stuff? It is really helpful for debugging derive with composite keys.

@udoprog
Copy link
Owner

udoprog commented Oct 14, 2022

Understood on the examples, but can you restore the one you deleted (map_feature) and just remove the feature stuff? It is really helpful for debugging derive with composite keys.

Ah. Not sure I want to have a permanent example for this since they're meant to be demonstrative uses of the library. 🤔 What if we keep it as a test? Then you can run it with cargo build --test <name>?

@udoprog
Copy link
Owner

udoprog commented Oct 14, 2022

The empty example should probably also be moved*!

@udoprog
Copy link
Owner

udoprog commented Oct 14, 2022

All right, with this you can now check all expansions with:

cargo install cargo-expand # if you don't already have it
cargo expand --test map_feature

Be warned: it's VERY noisy. :)

Does that work for you?

@udoprog udoprog merged commit 0751e25 into udoprog:main Oct 14, 2022
@udoprog udoprog mentioned this pull request Oct 14, 2022
6 tasks
@pitaj
Copy link
Collaborator Author

pitaj commented Oct 14, 2022

That works, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants