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

Specialize `MapForGrouping::fold` #873

Closed Philippe-Cholet closed 7 months ago

Philippe-Cholet commented 8 months ago

Related to #755

MapForGrouping is only internally used by GroupingMapBy and it only calls for_each on it (which in turn rely on fold). So I allow the clippy lint missing_trait_methods because specialize other methods is simply useless. (In case we run clippy with warning it.)

I could replace next body by unreachable!() and it would still work fine. Anyway, it will disappear from test coverage now that we have one (see here).

I previously wandered how to test and benchmark this:

Note that all GroupingMapBy methods ultimately rely on this fold so this specialization will improve performance for all its methods.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6814180) 94.38% compared to head (79f76be) 94.15%. Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #873 +/- ## ========================================== - Coverage 94.38% 94.15% -0.24% ========================================== Files 48 48 Lines 6665 6673 +8 ========================================== - Hits 6291 6283 -8 - Misses 374 390 +16 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

phimuemue commented 7 months ago

After a second thought, I'm pretty sure type MapForGrouping<I, F> = MapSpecialCase<I, MapForGroupingFn<F>> would be cleaner.

Philippe-Cholet commented 7 months ago

Seems cleaner indeed.