rust-itertools / itertools

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

Merge multipeek peeknth #940

Open Owen-CH-Leung opened 6 months ago

Owen-CH-Leung commented 6 months ago

As per #933 , this PR merges MultiPeek and PeekNth into a general private interface MultiPeekGeneral, with public type alias to define the compatible MultiPeek and PeekNth struct.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 78.26087% with 15 lines in your changes missing coverage. Please review.

Project coverage is 94.32%. Comparing base (6814180) to head (4fc8456). Report is 126 commits behind head on master.

Files with missing lines Patch % Lines
src/multipeek_general.rs 77.94% 15 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #940 +/- ## ========================================== - Coverage 94.38% 94.32% -0.07% ========================================== Files 48 48 Lines 6665 7013 +348 ========================================== + Hits 6291 6615 +324 - Misses 374 398 +24 ```

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

phimuemue commented 6 months ago

My suggestion: Deprecate multipeek and unify all the functionality into peek_nth.

Rationale: peek_nth is (advertised as) a drop-in replacement for std::iter::Peekable (nice for users), whereas multipeek offers exactly the same interface, but has subtle semantic differences (not nice for users if they are bitten by this).

And imho we should even un-fuse it to really be in line with std::iter::Peekable.

Philippe-Cholet commented 6 months ago

According to https://github.com/search?q=itertools+multipeek+reset_peek+lang%3ARust&type=code there is some real use of MultiPeek. Deprecate it seems excessive to me.

Maybe the status quo is not so bad.

I'm not opposed to unfuse it but it's a breaking change, I'd prefer to not break things for a while.

phimuemue commented 4 months ago

According to https://github.com/search?q=itertools+multipeek+reset_peek+lang%3ARust&type=code there is some real use of MultiPeek. Deprecate it seems excessive to me. [...] I'm not opposed to unfuse it but it's a breaking change, I'd prefer to not break things for a while.

Yes, we should not deprecate at a whim. But honestly I really like to know if deprecating wasn't for the better in the long run.

My rationale (and the linked examples appear to confirm this to some extent):

On top of that, I do not see a problem in keeping a deprecated MultiPeek and motivate users to go with the replacements (if we find them) in PeekNth.