Merge multipeek peeknth #940

Owen-CH-Leung · 2024-05-17T02:05:34Z

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.

… correct moduling

codecov · 2024-05-17T02:07:20Z

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 ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Philippe-Cholet

I reviewed but I'm not sure we should continue this, as merge implementations imply to merge method documentations, while the index behavior is different.
@phimuemue What do you think? Was I overzealous here?

Philippe-Cholet · 2024-05-17T06:46:10Z

src/lib.rs

    #[cfg(feature = "use_alloc")]
-    pub use crate::multipeek_impl::MultiPeek;
-    pub use crate::pad_tail::PadUsing;
+    pub use crate::multipeek_general::MultiPeek;
    #[cfg(feature = "use_alloc")]
-    pub use crate::peek_nth::PeekNth;
+    pub use crate::multipeek_general::PeekNth;


You can merge those imports.

Yup - merged.

Philippe-Cholet · 2024-05-17T06:50:17Z

src/multipeek_general.rs

+impl<I: Iterator, Idx: PeekIndex> PeekingNext for MultiPeekGeneral<I, Idx>
 where
    I: Iterator,


Nit: Iterator bound twice, move Idx bound below.

Thanks. Moved Idx bound below

Philippe-Cholet · 2024-05-17T06:51:09Z

src/multipeek_general.rs

 where
    I: Iterator,
 {
    fn peeking_next<F>(&mut self, accept: F) -> Option<Self::Item>
    where
        F: FnOnce(&Self::Item) -> bool,
    {
-        self.peek().filter(|item| accept(item))?;
+        self.peek_mut().filter(|item| accept(item))?;


I don't see why it would now need peek_mut.

Thanks. Reverted to peek

Philippe-Cholet · 2024-05-17T06:57:33Z

src/multipeek_general.rs

+    pub fn peek(&mut self) -> Option<&I::Item> {
+        let ret = if self.index < self.buf.len() {
+            Some(&self.buf[self.index])
+        } else {
+            match self.iter.next() {
+                Some(x) => {
+                    self.buf.push_back(x);
+                    Some(&self.buf[self.index])
+                }
+                None => return None,
+            }
+        };
+
+        self.index += 1;
+        ret
+    }
+}
+
+impl<I: Iterator> PeekNth<I> {
+    /// Works exactly like the `peek` method in [`std::iter::Peekable`].
+    pub fn peek(&mut self) -> Option<&I::Item> {
+        self.peek_nth(0)
+    }


I think it could be merged into MultiPeekGeneral::peek? With a PeekIndex::increment_index.

Yes - I've merged peek into MultiPeekGeneral and removed the impl block

Philippe-Cholet · 2024-05-17T07:06:18Z

src/multipeek_general.rs

+/// [`IntoIterator`] enabled version of [`crate::Itertools::multipeek`].
+pub fn multipeek<I>(iterable: I) -> MultiPeek<I::IntoIter>


To avoid crate:: to be user-visible in doc, we usually do:

[`Itertools::multipeek`](crate::Itertools::multipeek) which I usually prefer

(OR #[cfg(doc)] use crate::Itertools; at the top of the file to be able to only write [`Itertools::multipeek`] in the doc).

Thanks. I've revised to use

[`Itertools::multipeek`](crate::Itertools::multipeek)

Philippe-Cholet · 2024-05-17T07:16:25Z

src/multipeek_general.rs

+#![allow(private_interfaces)]
+#![allow(private_bounds)]


I discover those lints, or rather I probably usually fix what those lints tell me rather than allow them.

You are right. Turns out it passes the clippy even the lints are removed. I've removed them

Philippe-Cholet · 2024-05-17T07:16:57Z

src/free.rs

 #[cfg(feature = "use_alloc")]
-pub use crate::multipeek_impl::multipeek;
+pub use crate::multipeek_general::multipeek;
 #[cfg(feature = "use_alloc")]
-pub use crate::peek_nth::peek_nth;
+pub use crate::multipeek_general::peek_nth;


You can merge those lines.

Philippe-Cholet · 2024-05-17T07:19:14Z

src/multipeek_general.rs

+    pub iter: Fuse<I>,
+    pub buf: VecDeque<I::Item>,
+    pub index: Idx,


Almost missed this. The fields should remain private! If there is a need to access them from another module, you can pub(crate) the fields.

Thanks - I've changed them to private fields

Philippe-Cholet · 2024-05-17T07:25:55Z

src/multipeek_general.rs

+impl PeekIndex for () {
+    fn reset_index(&mut self) {}
+}


I mention elsewhere the need to add a fn increment_index(&mut self); but we probably need a more general fn add(&mut self, n: usize) { *self += n; } instead to update the index with the nth methods (and create tests about it).

Sure. I've revised increment_index to a generic add instead.

Philippe-Cholet · 2024-05-17T07:52:50Z

src/multipeek_general.rs

@@ -137,15 +165,64 @@ where
    {
        self.next_if(|next| next == expected)
    }
+
+    /// Works exactly like `next_if`, but for the `nth` value without advancing the iterator.
+    pub fn nth_if(&mut self, n: usize, func: impl FnOnce(&I::Item) -> bool) -> Option<&I::Item> {


What should we expect from MultiPeek::nth_if concerning the index? To advance only if func returned true I guess. But I would expect the doc of the method to decribe this but we can't make different docs for MultiPeek/PeekNth without making different methods.
⚠ Maybe I was too optimistic about this merge. ⚠

I believe with the merge we'd need to completely revise the doc to describe the API behaviour under MultiPeek and PeekNth. I can help to re-write them if we decide to go for the direction to merge both.

…h and MultiPeek

phimuemue · 2024-05-17T11:12:31Z

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 · 2024-05-17T16:29:21Z

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.

…peek

phimuemue · 2024-07-14T19:44:24Z

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):

People use MultiPeek even when there are simpler, more efficient alternatives: In particular, cloneable iterators. These could just be cloned - often more efficient than an allocating MultiPeek must allocate. So these programs do not need MultiPeek in the first place.
=> Maybe depretacing it or offering another API pushes users towards the better alternatives.
Where it's really needed: MultiPeek requires the users to be careful with the "cursor", so that callers sprinkle reset_peak because they need their iterator in a predictable state. I'm not sure that there's a simple API that avoids the problem, but I find this a recurring (and bad) pattern with this iterator.
=> Is there any real use case that does not conclude peeking with reset_peek? If not, can we offer an API that does not put this burden onto the callers?
As said, subtle difference between PeekNth and MultiPeek.
=> If prefer an API that makes this distinction more explicit.

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.

Owen-CH-Leung added 2 commits May 16, 2024 23:20

Add multipeek_general.rs

756ba95

Remove multipeek_impl and peek_nth, revise lib.rs and other files for…

c130144

… correct moduling

Owen-CH-Leung added 2 commits May 17, 2024 10:17

fix cargo fmt, cargo doc and rebase master

518c877

Merge branch 'master' into merge_multipeek_peeknth

5a1f1a9

Philippe-Cholet reviewed May 17, 2024

View reviewed changes

Owen-CH-Leung added 3 commits May 17, 2024 18:21

Address some of the comments

3fab1c2

Put peek into multipeek_general, and remove implementation for PeekNt…

4bf4e4b

…h and MultiPeek

Fix peeking_next

3920dbe

Change increment_index to generic add, add test to test peek_nth and …

4fc8456

…peek

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge multipeek peeknth #940

Merge multipeek peeknth #940

Owen-CH-Leung commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

Loading

Philippe-Cholet left a comment

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 18, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 17, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 18, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 19, 2024

Philippe-Cholet May 17, 2024

Owen-CH-Leung May 19, 2024

phimuemue commented May 17, 2024

Philippe-Cholet commented May 17, 2024

phimuemue commented Jul 14, 2024 •

edited

Loading

		/// [`IntoIterator`] enabled version of [`crate::Itertools::multipeek`].
		pub fn multipeek<I>(iterable: I) -> MultiPeek<I::IntoIter>

		#![allow(private_interfaces)]
		#![allow(private_bounds)]

Merge multipeek peeknth #940

Are you sure you want to change the base?

Merge multipeek peeknth #940

Conversation

Owen-CH-Leung commented May 17, 2024

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

Philippe-Cholet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phimuemue commented May 17, 2024

Philippe-Cholet commented May 17, 2024

phimuemue commented Jul 14, 2024 • edited Loading

codecov bot commented May 17, 2024 •

edited

Loading

phimuemue commented Jul 14, 2024 •

edited

Loading