Skip to content

Add a try_collect() helper method to Iterator #94041

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 18, 2022
Merged

Add a try_collect() helper method to Iterator #94041

merged 1 commit into from
Feb 18, 2022

Conversation

a-lafrance
Copy link
Contributor

Implement Iterator::try_collect() as a helper around Iterator::collect() as discussed here.

First time contributor so definitely open to any feedback about my implementation! Specifically wondering if I should open a tracking issue for the unstable feature I introduced.

As the main participant in the internals discussion: r? @scottmcm

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2022
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2022
@a-lafrance
Copy link
Contributor Author

Updated the try_collect() implementation to accept Try-not-FromIterator types like ControlFlow. Also modified the docs to get the main points of the method across better.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2022
@rust-log-analyzer

This comment has been minimized.

# to view
@scottmcm
Copy link
Member

scottmcm commented Feb 16, 2022 •
edited
Loading

That CI failure makes no sense. I'll try closing and reopening.

@scottmcm scottmcm closed this Feb 16, 2022
@scottmcm scottmcm reopened this Feb 16, 2022
@rust-log-analyzer

This comment has been minimized.

# to view
@scottmcm scottmcm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2022
@a-lafrance a-lafrance mentioned this pull request Feb 16, 2022
Open
6 tasks
@a-lafrance
Copy link
Contributor Author

Tracking issue: #94047

bjorn3
bjorn3 reviewed Feb 16, 2022
View reviewed changes
library/core/tests/iter/traits/iterator.rs

let u: Vec<Result<i32, ()>> = vec![Ok(1), Ok(2), Ok(3)];
let v = u.into_iter().try_collect::<Vec<i32>>();
assert_eq!(v, Ok(vec![1, 2, 3]));
Copy link
Member

@bjorn3 bjorn3 Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This one is already possible with .collect(). Thanks to impl<A, E, V> FromIterator<Result<A, E>> for Result<V, E> where V: FromIterator<A>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but the main benefits of try_collect() are increasing the discoverability of this technique and allowing Try-but-not-FromIterator types (e.g. ControlFlow) to be fallibly collected as well. See my comment on the tracking issue here for a more thorough description of the differences between try_collect() and collect().

@a-lafrance
Add a try_collect() helper method to Iterator
47d5196 
Tweaked `try_collect()` to accept more `Try` types

Updated feature attribute for tracking issue
@ehuss
Copy link
Contributor

ehuss commented Feb 16, 2022

The ICE in CI is caused by the use of intra-doc links involving core (the ones that are [`ControlFlow`]: core::ops::ControlFlow` and [`FromIterator`]: core::iter::FromIterator). There is more information in #73445. Obviously it shouldn't ICE and fall apart. I can't explain why that happens.

I'm pretty sure you can just reference them without qualification (like [`FromIterator`], no need for a reference) since they are in scope.

You can test documentation with ./x.py doc --stage 0 library/test. You can add the --open flag to open a web browser to inspect it.

@a-lafrance
Copy link
Contributor Author

Thanks for the info @ehuss! Just pushed a fix for the erring links, so @scottmcm I think it should be ready for a final review.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2022
@a-lafrance
Copy link
Contributor Author

Oh, and @rustbot label -S-blocked

@rustbot rustbot removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Feb 16, 2022
@scottmcm
Copy link
Member

scottmcm commented Feb 17, 2022 •
edited
Loading

Thank you, ehuss! And for the PR + updates, @a-lafrance!

This looks good to go into unstable by me. Lots more discussion to be had on the tracking issue, to be sure.
@bors r+

@bors
Copy link
Collaborator

bors commented Feb 17, 2022

📌 Commit 47d5196 has been approved by scottmcm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2022
@bors
Auto merge of rust-lang#94103 - matthiaskrgr:rollup-cd70ofn, r=matthi…
73a7423 
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#93337 (Update tracking issue numbers for inline assembly sub-features)
 - rust-lang#93758 (Improve comments about type folding/visiting.)
 - rust-lang#93780 (Generate list instead of div items in sidebar)
 - rust-lang#93976 (Add MAIN_SEPARATOR_STR)
 - rust-lang#94011 (Even more let_else adoptions)
 - rust-lang#94041 (Add a `try_collect()` helper method to `Iterator`)
 - rust-lang#94043 (Fix ICE when using Box<T, A> with pointer sized A)
 - rust-lang#94082 (Remove CFG_PLATFORM)
 - rust-lang#94085 (Clippy: Don't lint `needless_borrow` in method receiver positions)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a4be35e into rust-lang:master Feb 18, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 18, 2022
@a-lafrance a-lafrance deleted the try-collect branch February 18, 2022 02:11
@scottmcm scottmcm mentioned this pull request Feb 25, 2022
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@scottmcm scottmcm scottmcm left review comments

Assignees

@scottmcm scottmcm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@a-lafrance @rust-highfive @rust-log-analyzer @scottmcm @ehuss @bors @bjorn3 @rustbot