itertools
itertools copied to clipboard
More generic maps in `GroupingMap` (v2)
This an alternative to #901 that I learnt from, it would close #901, close #588 and eventually solve most https://github.com/rust-itertools/itertools/labels/generic-container issues.
- I reuse
Mappolished earlier. - Transform
GroupingMap[By]intoGroupingGenericMap[By]This is a huge but quite straightforward transformation:GroupingMap[By]becomesGroupingGenericMap[By]with a generic typeMK: HashbecomesM: Map<Key = K>HashMap<K, ...>becomesMwith aM: Map<Value = ...>bound- Remove
HashandHashMap use_stdbecomesuse_alloc
- Reintroduce
GroupingMap[By]with one more generic parameter This is a breaking change! However, this is mostly inferred. Note that it breaks (only) laziness tests because theGroupingMapobject is not used (I could write any type butu8is one of the possible returned types). If it was used then it would have been inferred. I copied/pasted the code of oldinto_grouping_map[_by]methods, added the genericRand updated each function body with_in(.., HashMap::new()). The old types are aliases of the new ones.
The main interest of this PR over #901 is that there is not _in versions for each GroupingMap method.
More than the additional parameter, the following code (weird but currently working) would not compile anymore.
let g = (0..10).into_grouping_map_by(|n| n % 2); // the map would now lives here so the values type is fixed earlier.
if true {
println!("{:?}", g.collect::<Vec<_>>()); // HashMap<i32, Vec<i32>>
} else {
println!("{:?}", g.sum()); // HashMap<i32, i32>
}
However, it's easily solvable here, .into_grouping_map_by(|n| n % 2) should be moved to both branches for the code to work as before.
Now, in other cases, it might be not that easily solvable! Is it a too big breaking change?
Codecov Report
Attention: Patch coverage is 66.66667% with 44 lines in your changes missing coverage. Please review.
Project coverage is 94.00%. Comparing base (
6814180) to head (2203cb6). Report is 149 commits behind head on master.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/generic_containers.rs | 33.33% | 42 Missing :warning: |
| src/grouping_map.rs | 95.34% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #906 +/- ##
==========================================
- Coverage 94.38% 94.00% -0.39%
==========================================
Files 48 49 +1
Lines 6665 6905 +240
==========================================
+ Hits 6291 6491 +200
- Misses 374 414 +40
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
To generalize the "quick tests", I would need to generalize into_group_map as well.
To avoid a very huge PR, I think that I should implement Map only for HashMap first then BTreeMap and only then Vec in following PRs.
@SkiFire13 I was able to make it work with a single additional generic parameter.
@phimuemue @jswrenn Do you think such breaking change is acceptable? I worry it might be too nasty for very few people.
I therefore searched on github and eventually found only one place this would break: they would be able to easily fix this by moving .into_grouping_map() into their two use cases.