Skip to content
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

Add itertools::note_position. #169

Merged
merged 2 commits into from
Nov 24, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub mod structs {
pub use intersperse::Intersperse;
pub use kmerge_impl::KMerge;
pub use pad_tail::PadUsing;
pub use note_position::NotePosition;
Copy link
Member

Choose a reason for hiding this comment

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

this line is off by one from being in alpha order.

pub use rciter_impl::RcIter;
pub use repeatn::RepeatN;
pub use sources::{RepeatCall, Unfold, Iterate};
Expand All @@ -84,6 +85,7 @@ pub use cons_tuples_impl::cons_tuples;
pub use diff::diff_with;
pub use diff::Diff;
pub use minmax::MinMaxResult;
pub use note_position::Position;
pub use repeatn::repeat_n;
pub use sources::{repeat_call, unfold, iterate};
pub use zip_longest::EitherOrBoth;
Expand All @@ -100,6 +102,7 @@ mod groupbylazy;
mod intersperse;
mod kmerge_impl;
mod minmax;
mod note_position;
mod pad_tail;
mod rciter_impl;
mod repeatn;
Expand Down Expand Up @@ -894,6 +897,27 @@ pub trait Itertools : Iterator {
adaptors::flatten(self)
}

/// Return an iterator adaptor that wraps each element in a `Position` to
/// ease special-case handling of the first or last elements.
///
/// Iterator element type is
/// [`Position<Self::Item>`](enum.Position.html)
///
/// ```
/// use itertools::{Itertools, Position};
///
/// let it = (0..4).note_position();
/// itertools::assert_equal(it, vec![Position::First(0), Position::Middle(1), Position::Middle(2), Position::Last(3)]);
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long, keep it inside 100 columns please.

///
/// let it = (0..1).note_position();
/// itertools::assert_equal(it, vec![Position::Only(0)]);
/// ```
fn note_position(self) -> NotePosition<Self>
where Self: Sized,
{
note_position::note_position(self)
}

// non-adaptor methods
/// Advances the iterator and returns the next items grouped in a tuple of
/// a specific size (up to 4).
Expand Down
92 changes: 92 additions & 0 deletions src/note_position.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use std::fmt;
use std::iter::Peekable;

/// An iterator adaptor that wraps each element in an [`Position`](../enum.Position.html).
///
/// Iterator element type is `Position<I::Item>`.
/// This iterator is *fused*.
Copy link
Member

Choose a reason for hiding this comment

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

The iterator is not fused. It doesn't need to be, just needs correct docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, although it doesn't use Fuse directly. Peekable is fused.

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 agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will remove the comment.

Copy link
Member

Choose a reason for hiding this comment

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

A fused iterator would pass the test called exact_position in this diff on top of your PR (as it was), and it doesn't. Peekable calls .next() on the underlying iterator, even if it has seen None already.

diff --git a/src/note_position.rs b/src/note_position.rs
index 58f8dc8..8bac7b1 100644
--- a/src/note_position.rs
+++ b/src/note_position.rs
@@ -89,4 +89,8 @@ impl<I: Iterator> Iterator for NotePosition<I> {
             (_, None) => None,
         }
     }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        self.peekable.size_hint()
+    }
 }
diff --git a/tests/quick.rs b/tests/quick.rs
index a0176c8..9aa8681 100644
--- a/tests/quick.rs
+++ b/tests/quick.rs
@@ -346,6 +346,12 @@ quickcheck! {
     fn exact_interleave(a: Iter<i16>, b: Iter<i16>) -> bool {
         exact_size_for_this(a.interleave(b))
     }
+    fn exact_position(a: Iter<i16>) -> bool {
+        exact_size_for_this(a.note_position())
+    }
+    fn exact_position_2(a: Vec<i16>) -> bool {
+        exact_size_for_this(a.iter().note_position())
+    }
     fn size_interleave_shortest(a: Iter<i16>, b: Iter<i16>) -> bool {
         correct_size_hint(a.interleave_shortest(b))
     }

(Iter is a non-fused iterator that the quickcheck test suite uses).

Copy link
Member

Choose a reason for hiding this comment

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

hm ok, after some thinking about peekable, it's actually pretty bad that it's not fused.

Calling .peek() and observing None is the only signal the user gets that the iterator is exhausted -- calling .next() after that leads to iterator-specific behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yuck. FWIW, I think I misread the std docs, which say impl<I> FusedIterator for Peekable<I> where I: FusedIterator, which makes sense.

Should I change this to Peekable<Fuse<I>> to work around that problem for now?

Copy link
Member

Choose a reason for hiding this comment

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

yes

///
/// See [`.note_position()`](../trait.Itertools.html#method.note_position) for more information.
pub struct NotePosition<I>
where I: Iterator,
{
handled_first: bool,
peekable: Peekable<I>,
}

/// Create a new `NotePosition` iterator.
pub fn note_position<I>(iter: I) -> NotePosition<I>
where I: Iterator,
{
NotePosition {
handled_first: false,
peekable: iter.peekable(),
}
}

/// A value yielded by `NotePosition`.
/// Indicates the position of this element in the iterator results.
///
/// See [`.note_position()`](trait.Itertools.html#method.note_position) for more information.
pub enum Position<T> {
Copy link
Member

Choose a reason for hiding this comment

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

derive(Copy, Clone) are appropriate.

/// This is the first element.
First(T),
/// This is neither the first nor the last element.
Middle(T),
/// This is the last element.
Last(T),
/// This is the only element.
Only(T),
}

impl<T: PartialEq> PartialEq for Position<T> {
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to derive(PartialEq), prefer using 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.

I did not know you could derive(PartialEq) or Debug on a generic without having a trait bound for it. TIL!

Copy link
Member

Choose a reason for hiding this comment

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

Now you know what the deriving recipes do: literally what you typed :)

fn eq(&self, other: &Position<T>) -> bool {
match (self, other) {
(&Position::First(ref a), &Position::First(ref b)) => a == b,
(&Position::Middle(ref a), &Position::Middle(ref b)) => a == b,
(&Position::Last(ref a), &Position::Last(ref b)) => a == b,
(&Position::Only(ref a), &Position::Only(ref b)) => a == b,
_ => false,
}
}
}

impl<T: fmt::Debug> fmt::Debug for Position<T> {
Copy link
Member

Choose a reason for hiding this comment

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

This is equivalent to derive(Debug) except for pretty printing, why not use deriving.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
&Position::First(ref a) => write!(f, "First({:?})", a),
&Position::Middle(ref a) => write!(f, "Middle({:?})", a),
&Position::Last(ref a) => write!(f, "Last({:?})", a),
&Position::Only(ref a) => write!(f, "Only({:?})", a),
}
}
}

impl<I: Iterator> Iterator for NotePosition<I> {
type Item = Position<I::Item>;

fn next(&mut self) -> Option<Self::Item> {
match (self.handled_first, self.peekable.next()) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if this method was written in a style where there's a branch if self.handled_first inside the match/conditional on .next()

// Haven't seen the first item yet, and there is one to give.
(false, Some(item)) => {
self.handled_first = true;
// Peek to see if this is also the last item,
// in which case tag it as `Only`.
match self.peekable.peek() {
Some(_) => Some(Position::First(item)),
None => Some(Position::Only(item)),
}
}
// Have seen the first item, and there's something left.
(true, Some(item)) => {
// Peek to see if this is the last item.
match self.peekable.peek() {
Some(_) => Some(Position::Middle(item)),
None => Some(Position::Last(item)),
}
}
// Iterator is finished.
(_, None) => None,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should forward size_hint from the underlying iterator.

}