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

Feature process_results as an Itertools method #587

Closed
Closed
Changes from all commits
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
44 changes: 44 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,50 @@ pub trait Itertools : Iterator {
adaptors::map_ok(self, f)
}

/// “Lift” a function of the values of the current iterator so as to process
/// an iterator of `Result` values instead.
///
/// `processor` is a closure that receives an adapted version of the iterator
/// as the only argument — the adapted iterator produces elements of type `Ok`,
/// as long as the original iterator produces `Ok` values.
Comment on lines +870 to +872
Copy link
Member

Choose a reason for hiding this comment

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

This documentation differs slightly from what we have at https://docs.rs/itertools/0.10.3/itertools/fn.process_results.html, and I think, the choice of Ok is misleading, as it hints at a Result - https://docs.rs/itertools/0.10.3/itertools/fn.process_results.html goes with T, I suggest copying that to have it in line.

///
/// If the original iterable produces an error at any point, the adapted
/// iterator ends (short-circuits) and the `process_results` function will
/// return the error iself.
///
/// Otherwise, the return value from the closure is returned wrapped
/// inside `Ok`.
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 misleading, as this Ok refers to Result::Ok (and not the local type Ok). Or am I missing something?

///
/// # Example
///
/// ```
/// use itertools::Itertools;
///
/// type Item = Result<i32, &'static str>;
///
/// let first_values: Vec<Item> = vec![Ok(1), Ok(0), Ok(3)];
/// let second_values: Vec<Item> = vec![Ok(2), Ok(1), Err("overflow")];
///
/// // “Lift” the iterator .max() method to work on the Ok-values.
/// let first_max = first_values.into_iter().process_results(|iter| iter.max().unwrap_or(0));
/// let second_max = second_values.into_iter().process_results(|iter| iter.max().unwrap_or(0));
///
/// assert_eq!(first_max, Ok(3));
/// assert!(second_max.is_err());
/// ```
///
/// This is the iterator adaptor version of [`itertools::process_results()`].
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

///
/// [`itertools::process_results`]: `crate::process_results`
fn process_results<F, Ok, Err, R>(self, processor: F) -> Result<R, Err>
where Self: Iterator<Item = Result<Ok, Err>> + Sized,
F: for<'r> FnOnce(ProcessResults<'r, Self, Err>) -> R,
Copy link
Member

Choose a reason for hiding this comment

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

Is the higher kinded lifetime (i.e. for<'r>) necessary?

// Added to directly highlight the relevant impl in the documentation
for<'r> ProcessResults<'r, Self, Err>: Iterator<Item = Ok>,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? If it's only for documentation, I'd prefer to remove it and not bother the type checker with it. If we really want it in the documentation, we can still state it explicitly.

{
process_results(self, processor)
}

/// Return an iterator adaptor that filters every `Result::Ok`
/// value with the provided closure. `Result::Err` values are
/// unchanged.
Expand Down