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

Adds keys and values iterators #61

Merged
merged 3 commits into from
Jul 10, 2020
Merged

Adds keys and values iterators #61

merged 3 commits into from
Jul 10, 2020

Conversation

mlodato517
Copy link
Contributor

@mlodato517 mlodato517 commented Jul 9, 2020

This PR
Adds keys and values iterator helpers to the MapReadRef per this comment.


This change is Reviewable

tests/lib.rs Outdated Show resolved Hide resolved
src/read/read_ref.rs Show resolved Hide resolved
src/read/read_ref.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #61 into master will increase coverage by 0.76%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   82.78%   83.54%   +0.76%     
==========================================
  Files          12       12              
  Lines        1185     1234      +49     
==========================================
+ Hits          981     1031      +50     
+ Misses        204      203       -1     
Impacted Files Coverage Δ
src/read/read_ref.rs 76.47% <100.00%> (+6.10%) ⬆️
tests/lib.rs 98.15% <100.00%> (+0.10%) ⬆️
tests/quick.rs 93.38% <100.00%> (+1.15%) ⬆️
src/write.rs 74.32% <0.00%> (-0.10%) ⬇️
src/read/mod.rs 78.40% <0.00%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29db7a6...61c6642. Read the comment docs.

src/read/read_ref.rs Outdated Show resolved Hide resolved
@mlodato517 mlodato517 marked this pull request as ready for review July 10, 2020 21:55
@jonhoo
Copy link
Owner

jonhoo commented Jul 10, 2020

This looks great once again, thank you!

@jonhoo jonhoo merged commit cde2f7d into jonhoo:master Jul 10, 2020
@mlodato517 mlodato517 deleted the implement-keys-and-values-iterators branch July 10, 2020 21:59
@mlodato517
Copy link
Contributor Author

You're welcome! And thanks for all your great reviews!

While writing these assertions I was thinking, "It might be convenient to implement Eq on Values if the inner value implements Eq". But I can't tell if it's just convenient for tests. Is that something you might want?

Otherwise, looking through the indexmap tests there are tests for map.values_mut(), map1 == map2, and then some stuff based on sort order that I'm guessing we don't care about because we don't care about order. Would you prefer that kind of functionality?

Above and beyond all that, are there any other things you've been looking to revisit/clean up that I could tackle? This comment looks like an obvious place to start. I could think on that if it's valuable. I'm not sure I'd be able to brain out something at this level, but I'd also be interested in trying 🤷

Or if there are any other projects you have or know about that need some more chores like this done (or easy features or whatever), I'm game!

@jonhoo
Copy link
Owner

jonhoo commented Jul 10, 2020

It's been a pleasure!

I think PartialEq + Eq on Values is probably mostly useful for tests, but it seems reasonable enough to me! One thing that'll be tricky is to check that the cardinality is right if one thing is Short (but has duplicates) and the other thing is a Long.

I think the particular code comment you linked to is unlikely to matter much in practice, since it only every runs once for a given evmap. In general, a good strategy is to look for things that are missing in the coverage map and add tests for those. In terms of other issues, it would be awesome to add loom tests (#17), which will also require some thinking to get right.

As far as other projects go, here are a couple:

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

Successfully merging this pull request may close these issues.

2 participants