Add Iterator::try_find #63177
Merged
Add Iterator::try_find #63177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 tasks
|
Sorry about force-pushes. The code was completely broken, but not it compiles at least, so I'll be adding commits instead of force pushing from now on. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The iterator trait is generally not built around iteration over Results, and I'm not sure that adding a variant of a single method makes sense when thinking about Iterator on the whole. Thoughts @rust-lang/libs? |
|
We already have |
|
Using |
|
I also had a feeling this |
|
@alexcrichton could you elaborate more? If I understand correctly - stabilizing means promoting this fn to stable, and I'd rather just keep unstable for a while - cause maybe there's a better way of providing this API. I fully agree that we might want to add other |
|
Sorry yeah what I mean to say is that I would want to pause before stabilization, but this is fine to land unstable at any time. When landing it unstable though it seem reasonable to try to use |
|
Right, I'm in progress with switching to |
fn try_find<F, E, T, FR, R>(&mut self, mut f: F) -> R where
Self: Sized,
FR: Try<Ok = bool, Error = E>,
F: FnMut(&Self::Item) -> FR,
R: Try<Ok = Option<Self::Item>, Error = E> { ... }This is what I'm trying atm. Is it ok if we use |
|
Returning the Any thoughts? |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'm having troubles with running tests locally - UPD: for later: |
|
I really like this, having it return a Something else to think about: similar to |
|
I think, as a general rule, almost anything that takes With the result - I think I implement it with |
|
I've created this: https://github.com/MOZGIII/iter-try-rs Based on this experiment, I think it looks like we should only provide a single implementation of the I'd suggest to go with the |
|
☔ The latest upstream changes (presumably #67596) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? @Amanieu |
|
@MOZGIII if you can resolve the conflicts, we can proceed with this |
Dylan-DPC-zz
added
S-waiting-on-author
Amanieu
reviewed
Dec 31, 2019
|
Rebased, ready to proceed |
|
Run |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Thx, done |
|
@bors r+ |
|
📌 Commit 5446cc9 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 2, 2020
Add Iterator::try_find I found a need for this fn, and created this PR. Tracking issue: #63178 I did a fair amount of thinking about the function name, and settled on the current one. I don't see other anything else that's non-trivial here, but I'm open for debate. I just want this functionality to be there. It couples with the `collect` trick for collecting `Result<Vec<T>, E>` from `Iterator<Item = Result<T, E>>`. UPD: I've already looked at `fallible_iterator` crate, but I don't think it supports my use case. The main problem is that I can't construct a failable iterator. I have a regular iterator, and I just need to apply a predicate that can fail via `find` method. UPD: `fallible_iterator` would work, but it's not elegant cause I'd have to make a failable iterator by mapping iterator with `Result::Ok` first.
|
☀️ Test successful - checks-azure |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I found a need for this fn, and created this PR.
Tracking issue: #63178
I did a fair amount of thinking about the function name, and settled on the current one.
I don't see other anything else that's non-trivial here, but I'm open for debate. I just want this functionality to be there.
It couples with the
collecttrick for collectingResult<Vec<T>, E>fromIterator<Item = Result<T, E>>.UPD:
I've already looked at
fallible_iteratorcrate, but I don't think it supports my use case.The main problem is that I can't construct a failable iterator. I have a regular iterator, and I just need to apply a predicate that can fail via
findmethod.UPD:
fallible_iteratorwould work, but it's not elegant cause I'd have to make a failable iterator by mapping iterator withResult::Okfirst.