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 circular_tuple_windows #350
Conversation
5dd57af
to
4b2f48b
Compare
4b2f48b
to
f20ea30
Compare
/// itertools::assert_equal(it, vec![(1, 2, 3), (2, 3, 4), (3, 4, 1), (4, 1, 2)]); | ||
/// ``` | ||
fn circular_tuple_windows<T>(self) -> CircularTupleWindows<Self, T> | ||
where Self: Sized + Clone + Iterator<Item = T::Item> + ExactSizeIterator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any issues with eliminating the ExactSizeIterator
bound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added ExactSizeIterator
so that the call to len()
on line 198 on in tuple_impl.rs
would be performant. Using tuple_windows(iter.cycle()).take(len)
was a neat way of getting the extra elements wrapped onto the end, but if removing the ExactSizeIterator
constraint is preferable we could potentially go with something like the following, at the expense of a slight reduction in readability:
pub struct CircularTupleWindows<I, T: Clone>
where I: Iterator<Item = T::Item> + Clone,
T: TupleCollect + Clone,
{
iter: TupleWindows<Chain<Chain<IntoIter<<T as TupleCollect>::Item>, I>, IntoIter<<T as TupleCollect>::Item>>, T>,
}
pub fn circular_tuple_windows<I, T>(mut iter: I) -> CircularTupleWindows<I, T>
where I: Iterator<Item = T::Item> + Clone,
T: TupleCollect + Clone,
T::Item: Clone,
{
let first_elements = iter.by_ref().take(<T as TupleCollect>::num_items() - 1).collect::<Vec<_>>();
let iter = tuple_windows(first_elements.clone().into_iter().chain(iter).chain(first_elements.into_iter()));
CircularTupleWindows {
iter: iter,
}
}
impl<I, T> Iterator for CircularTupleWindows<I, T>
where I: Iterator<Item = T::Item> + Clone,
T: TupleCollect + Clone,
T::Item: Clone,
{
type Item = T;
fn next(&mut self) -> Option<T> {
self.iter.next()
}
}
what do you think?
Any update on this? :) |
f20ea30
to
db42251
Compare
Just waiting on feedback from @jswrenn on which way to go regarding the |
Sorry for dropping the ball on this! I think I can merge this as-is. There's no reason we can't drop the bound later if it's unduly prohibitive. bors r+ |
350: Add circular_tuple_windows r=jswrenn a=ed-bassett As described in #349, I think `circular_tuple_windows` would be broadly useful enough to be added to Itertools. It relies on `tuple_windows` and cloning, so I'm sure it could not be included in the standard library. I'm not sure if this is the best way to implement the function, but I thought I would give it a shot so that at the very least you'd have something functional to work from. Co-authored-by: Ed Bassett <ed@devsketchpad.com>
Any chance I can squash in the fixup before it's merged? I was holding off while it was waiting on review, but I don't really know the process around getting things merged |
Sure! Just let me know when you're ready. |
db42251
to
e605002
Compare
Good to go |
bors r+ |
Build succeeded |
As described in #349, I think
circular_tuple_windows
would be broadly useful enough to be added to Itertools. It relies ontuple_windows
and cloning, so I'm sure it could not be included in the standard library.I'm not sure if this is the best way to implement the function, but I thought I would give it a shot so that at the very least you'd have something functional to work from.