Feature process_results as an Itertools method
#587
Conversation
| /// `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. |
There was a problem hiding this comment.
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.
| /// return the error iself. | ||
| /// | ||
| /// Otherwise, the return value from the closure is returned wrapped | ||
| /// inside `Ok`. |
There was a problem hiding this comment.
This is misleading, as this Ok refers to Result::Ok (and not the local type Ok). Or am I missing something?
| /// assert!(second_max.is_err()); | ||
| /// ``` | ||
| /// | ||
| /// This is the iterator adaptor version of [`itertools::process_results()`]. |
| /// [`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, |
There was a problem hiding this comment.
Is the higher kinded lifetime (i.e. for<'r>) necessary?
| where Self: Iterator<Item = Result<Ok, Err>> + Sized, | ||
| F: for<'r> FnOnce(ProcessResults<'r, Self, Err>) -> R, | ||
| // Added to directly highlight the relevant impl in the documentation | ||
| for<'r> ProcessResults<'r, Self, Err>: Iterator<Item = Ok>, |
There was a problem hiding this comment.
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.
591: Canonicalize free functions r=jswrenn a=phimuemue As made visible by #587, I think we should have (somewhat) uniform documentation of free functions. On top of that, I think `fn equal` should actually use `Iterator::eq` (available since Rust 1.5), instead of only referring to it in the documentation. Maybe someone finds time to have a look at this, as I'd like to avoid silly mistakes before merging. Co-authored-by: philipp <descpl@yahoo.de>
|
@danielhenrymantilla Thanks for this work, Daniel. Gentle nudge. I'd love to see this PR merged. |
680: Feature `process_results` as an `Itertools` method r=phimuemue a=Philippe-Cholet Fixes #557 I hope it's okay for me to propose an alternative pull request to #587 because it seems stuck for some time: misleading generics names Ok/Err, unnecessary ? higher kinded lifetime. I will be around to change things if necessary, the documentation maybe, what should I write? Co-authored-by: Philippe-Cholet <phcholet@orange.fr>
|
Thanks for bringing this up. We took the idea, but went with the somewhat simpler #680 |
Fixes #557