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 .flatten_ok() #527

Merged
merged 7 commits into from Apr 21, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/flatten_ok.rs
@@ -0,0 +1,61 @@
pub fn flatten_ok<I, T, E>(iter: I) -> FlattenOk<I, T, E>
where
I: Iterator<Item = Result<T, E>>,
T: IntoIterator,
{
FlattenOk { iter, inner: None }
}

/// An iterator adaptor that flattens `Result::Ok` values and
/// allows `Result::Err` values through unchanged.
///
/// See [`.flatten_ok()`](crate::Itertools::flatten_ok) for more information.
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
pub struct FlattenOk<I, T, E>
where
I: Iterator<Item = Result<T, E>>,
T: IntoIterator,
{
iter: I,
inner: Option<T::IntoIter>,
}

impl<I, T, E> Iterator for FlattenOk<I, T, E>
where
I: Iterator<Item = Result<T, E>>,
T: IntoIterator,
{
type Item = Result<T::Item, E>;

fn next(&mut self) -> Option<Self::Item> {
loop {
if let Some(inner) = &mut self.inner {
if let Some(item) = inner.next() {
return Some(Ok(item));
} else {
self.inner = None;
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need this else? The only case where it isn't executed is if self.inner is Some(_) and inner.next() is also Some(_), but in that case you have an early return. I would execute the contents of the else in any case, both if the if let succeeds or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SkiFire13 Your point is sound. In fact, we shouldn't even need to assign it to None in that case as it will just automatically fall through. That might hurt performance in the case where next() is called on an empty iterator repeatedly, but I don't think that actually matters. So yes, let me make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think setting it to None might have a reason if you consider consistency in case next is called after the iterator ended. Should it try to advance the last inner iterator? Or the outer iterator? Currently the stdlib fuse both of them, but I guess it would be reasonable if you advance the outer iterator (in that case you need to set the self.inner to None)

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 realize now that I should not change setting the inner to None as it might incur a performance penalty when hitting several Err values in a row.

match self.iter.next() {
Some(Ok(ok)) => self.inner = Some(ok.into_iter()),
Some(Err(e)) => return Some(Err(e)),
None => return None,
}
}
}
}
}

impl<I, T, E> Clone for FlattenOk<I, T, E>
where
I: Iterator<Item = Result<T, E>> + Clone,
T: IntoIterator,
T::IntoIter: Clone,
{
fn clone(&self) -> Self {
Self {
iter: self.iter.clone(),
inner: self.inner.clone(),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You could use clone_fields for consistency here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed and will push shortly.

}
26 changes: 26 additions & 0 deletions src/lib.rs
Expand Up @@ -119,6 +119,7 @@ pub mod structs {
pub use crate::cons_tuples_impl::ConsTuples;
pub use crate::exactly_one_err::ExactlyOneError;
pub use crate::format::{Format, FormatWith};
pub use crate::flatten_ok::FlattenOk;
#[cfg(feature = "use_std")]
pub use crate::grouping_map::{GroupingMap, GroupingMapBy};
#[cfg(feature = "use_alloc")]
Expand Down Expand Up @@ -194,6 +195,7 @@ mod combinations;
mod combinations_with_replacement;
mod exactly_one_err;
mod diff;
mod flatten_ok;
mod format;
#[cfg(feature = "use_std")]
mod grouping_map;
Expand Down Expand Up @@ -833,6 +835,30 @@ pub trait Itertools : Iterator {
adaptors::filter_map_ok(self, f)
}

/// Return an iterator adaptor that flattens every `Result::Ok` value into
/// a series of `Result::Ok` values. `Result::Err` values are unchanged.
///
/// This is useful when you have some common error type for your crate and
/// need to propogate it upwards, but the `Result::Ok` case needs to be flattened.
///
/// ```
/// use itertools::Itertools;
///
/// let input = vec![Ok(0..2), Err(false), Ok(2..4)];
/// let it = input.iter().cloned().flatten_ok();
/// itertools::assert_equal(it.clone(), vec![Ok(0), Ok(1), Err(false), Ok(2), Ok(3)]);
///
/// // This can also be used to propogate errors when collecting.
/// let output_result: Result<Vec<i32>, bool> = it.collect();
/// assert_eq!(output_result, Err(false));
/// ```
fn flatten_ok<T, E>(self) -> FlattenOk<Self, T, E>
where Self: Iterator<Item = Result<T, E>> + Sized,
T: IntoIterator
{
flatten_ok::flatten_ok(self)
}

/// Return an iterator adaptor that merges the two base iterators in
/// ascending order. If both base iterators are sorted (ascending), the
/// result is sorted.
Expand Down