rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.71k stars 309 forks source link

More generic maps in `GroupingMap` (v2) #906

Open Philippe-Cholet opened 6 months ago

Philippe-Cholet commented 6 months ago

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.

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[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 94.00%. Comparing base (6814180) to head (2203cb6). Report is 41 commits behind head on master.

Files 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 ```diff @@ 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.

Philippe-Cholet commented 6 months ago

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.