Skip to content

Merge multipeek peeknth #940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions src/free.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ pub use crate::adaptors::{interleave, put_back};
pub use crate::kmerge_impl::kmerge;
pub use crate::merge_join::{merge, merge_join_by};
#[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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged

#[cfg(feature = "use_alloc")]
pub use crate::put_back_n_impl::put_back_n;
#[cfg(feature = "use_alloc")]
Expand Down
12 changes: 5 additions & 7 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ pub mod structs {
pub use crate::kmerge_impl::{KMerge, KMergeBy};
pub use crate::merge_join::{Merge, MergeBy, MergeJoinBy};
#[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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge those imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - merged.

pub use crate::pad_tail::PadUsing;
pub use crate::peeking_take_while::PeekingTakeWhile;
#[cfg(feature = "use_alloc")]
pub use crate::permutations::Permutations;
Expand Down Expand Up @@ -205,10 +205,8 @@ mod lazy_buffer;
mod merge_join;
mod minmax;
#[cfg(feature = "use_alloc")]
mod multipeek_impl;
mod multipeek_general;
mod pad_tail;
#[cfg(feature = "use_alloc")]
mod peek_nth;
mod peeking_take_while;
#[cfg(feature = "use_alloc")]
mod permutations;
Expand Down Expand Up @@ -4115,7 +4113,7 @@ pub trait Itertools: Iterator {
where
Self: Sized,
{
multipeek_impl::multipeek(self)
multipeek_general::multipeek(self)
}

/// Collect the items in this iterator and return a `HashMap` which
Expand Down
121 changes: 99 additions & 22 deletions src/peek_nth.rs → src/multipeek_general.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,38 @@
use crate::size_hint;
use crate::PeekingNext;
#![allow(private_interfaces)]
#![allow(private_bounds)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


use crate::{size_hint, PeekingNext};
use alloc::collections::VecDeque;
use std::iter::Fuse;

/// See [`peek_nth()`] for more information.
#[derive(Clone, Debug)]
/// See [`multipeek()`] for more information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove, MultiPeekGeneral is not MultiPeek.

#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
pub struct PeekNth<I>
#[derive(Debug, Clone)]
pub struct MultiPeekGeneral<I: Iterator, Idx> {
pub iter: Fuse<I>,
pub buf: VecDeque<I::Item>,
pub index: Idx,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I've changed them to private fields

}

/// See [`multipeek()`] for more information.
pub type MultiPeek<I> = MultiPeekGeneral<I, usize>;

/// See [`peek_nth()`] for more information.
pub type PeekNth<I> = MultiPeekGeneral<I, ()>;

/// An iterator adaptor that allows the user to peek at multiple `.next()`
/// values without advancing the base iterator.
///
/// [`IntoIterator`] enabled version of [`crate::Itertools::multipeek`].
pub fn multipeek<I>(iterable: I) -> MultiPeek<I::IntoIter>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've revised to use

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

where
I: Iterator,
I: IntoIterator,
{
iter: Fuse<I>,
buf: VecDeque<I::Item>,
MultiPeek {
iter: iterable.into_iter().fuse(),
buf: VecDeque::new(),
index: 0,
}
}

/// A drop-in replacement for [`std::iter::Peekable`] which adds a `peek_nth`
Expand All @@ -28,18 +49,25 @@
PeekNth {
iter: iterable.into_iter().fuse(),
buf: VecDeque::new(),
index: (),
}
}

impl<I> PeekNth<I>
where
I: Iterator,
{
/// Works exactly like the `peek` method in [`std::iter::Peekable`].
pub fn peek(&mut self) -> Option<&I::Item> {
self.peek_nth(0)
pub trait PeekIndex {
fn reset_index(&mut self);
}

impl PeekIndex for () {
fn reset_index(&mut self) {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl PeekIndex for usize {
fn reset_index(&mut self) {
*self = 0;
}
}

impl<I: Iterator, Idx: PeekIndex> MultiPeekGeneral<I, Idx> {
/// Works exactly like the `peek_mut` method in [`std::iter::Peekable`].
pub fn peek_mut(&mut self) -> Option<&mut I::Item> {
self.peek_nth_mut(0)
Expand Down Expand Up @@ -137,15 +165,64 @@
{
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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

match self.peek_nth(n) {
Some(item) if func(item) => Some(item),
_ => None,

Check warning on line 173 in src/multipeek_general.rs

View check run for this annotation

Codecov / codecov/patch

src/multipeek_general.rs#L170-L173

Added lines #L170 - L173 were not covered by tests
}
}

Check warning on line 175 in src/multipeek_general.rs

View check run for this annotation

Codecov / codecov/patch

src/multipeek_general.rs#L175

Added line #L175 was not covered by tests

/// Works exactly like `next_if_eq`, but for the `nth` value without advancing the iterator.
pub fn nth_if_eq<T>(&mut self, n: usize, expected: &T) -> Option<&I::Item>
where
T: ?Sized,
I::Item: PartialEq<T>,
{
self.nth_if(n, |item| item == expected)
}

Check warning on line 184 in src/multipeek_general.rs

View check run for this annotation

Codecov / codecov/patch

src/multipeek_general.rs#L178-L184

Added lines #L178 - L184 were not covered by tests
}
impl<I: Iterator> MultiPeek<I> {
/// Reset the peeking “cursor”
pub fn reset_peek(&mut self) {
self.index = 0
}

impl<I> Iterator for PeekNth<I>
where
I: Iterator,
{
/// Works exactly like `.next()` with the only difference that it doesn't
/// advance itself. `.peek()` can be called multiple times, to peek
/// further ahead.
/// When `.next()` is called, reset the peeking “cursor”.
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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

impl<I: Iterator, Idx: PeekIndex> Iterator for MultiPeekGeneral<I, Idx> {
type Item = I::Item;

fn next(&mut self) -> Option<Self::Item> {
self.index.reset_index();
self.buf.pop_front().or_else(|| self.iter.next())
}

Expand All @@ -162,17 +239,17 @@
}
}

impl<I> ExactSizeIterator for PeekNth<I> where I: ExactSizeIterator {}
impl<I: ExactSizeIterator, Idx: PeekIndex> ExactSizeIterator for MultiPeekGeneral<I, Idx> {}

impl<I> PeekingNext for PeekNth<I>
impl<I: Iterator, Idx: PeekIndex> PeekingNext for MultiPeekGeneral<I, Idx>
where
I: Iterator,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Iterator bound twice, move Idx bound below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Moved Idx bound below

{
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))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Reverted to peek

self.next()
}
}
116 changes: 0 additions & 116 deletions src/multipeek_impl.rs

This file was deleted.

Loading