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 Iterator::collect_into #93057

Merged
merged 2 commits into from
Mar 10, 2022
Merged

Add Iterator::collect_into #93057

merged 2 commits into from
Mar 10, 2022

Conversation

frengor
Copy link
Contributor

@frengor frengor commented Jan 19, 2022

This PR adds Iterator::collect_into as proposed by @cormacrelf in #48597 (see #48597 (comment)).
Followup of #92982.

This adds the following method to the Iterator trait:

fn collect_into<E: Extend<Self::Item>>(self, collection: &mut E) -> &mut E

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (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 Jan 19, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I've skimmed the previous discussions in #48597 and #92982 and I don't personally see the appeal of this. Adding a minor variation like this to accomplish something that is already easy distracts people into having to think about which way to write the same thing when, rather than just writing what they mean, and that overshadows any benefit of having the API.

Is there someone else from @rust-lang/libs-api who would prefer to see this signature added?

fn collect_into<E: Extend<Self::Item>>(self, collection: &mut E) -> &mut E

If so, I'll let you approve the unstable and we can hash it out further when it comes to stabilization.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 28, 2022

I'm not entirely convinced, but I do see how it makes Extend more discoverable by making collect_into one of the possible options at the end of a iterator method chain. Just like how .collect() is much more discoverable than FromIter, even though Collection::from_iter(..) can often result in less verbose code.

(I don't think collect_into() should return anything though.)

Some relevant grep counts from the crates on crates.io:

collect:   256_874
from_iter:   9_225
extend:     57_110

Extend also has some useful implementations that might be nice to make more discoverable, such as extending a pair of two collections at once ('unzip').

(That particular implementation would be more powerful if we had impl<E: Extend> Extend for &mut E, which is technically a breaking change and was therefore rejected four years ago. However, I'm unable to find any code that would break on crates.io, which makes me wonder if we can still decide to add that implementation.)

@frengor
Copy link
Contributor Author

frengor commented Jan 28, 2022

Adding a minor variation like this to accomplish something that is already easy distracts people into having to think about which way to write the same thing when, rather than just writing what they mean, and that overshadows any benefit of having the API.

I personally don't think collect_into will distract people from writing what they mean, since this also applies to collect: .collect() lets Collection::from_iter(...) to be called from the iterator as well as .collect_into(...) does with Collection::extend(...).
However, I presume nobody thinks too much about the method to choose between .collect() and Collection::from_iter(...).

Moreover, as pointed out by @m-ou-se, when learning about iterators, collect makes beginners discover FromIterator. However, this does not apply to Extend. I think also Extend should deserve its method.

Finally, I think people will just use the method they prefer. Also note that the proposed doc is clearly stating that collect_into is a convenience method:

This method is a convenience method to call Extend::extend,
but instead of being called on a collection, it's called on an iterator.

@bors
Copy link
Contributor

bors commented Feb 4, 2022

☔ The latest upstream changes (presumably #93645) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 18, 2022

☔ The latest upstream changes (presumably #94103) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 9, 2022

Alright, let's try this out as an unstable feature.

Can you open a tracking issue and put its number it in the #[unstable] attribute? Thanks!

@m-ou-se m-ou-se 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 Mar 9, 2022
@frengor
Copy link
Contributor Author

frengor commented Mar 9, 2022

Can you open a tracking issue and put its number it in the #[unstable] attribute?

Of course, give me a moment.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 9, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 9, 2022

📌 Commit 63eddb3 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 9, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 9, 2022
Add Iterator::collect_into

This PR adds `Iterator::collect_into` as proposed by `@cormacrelf` in rust-lang#48597 (see rust-lang#48597 (comment)).
Followup of rust-lang#92982.

This adds the following method to the Iterator trait:

```rust
fn collect_into<E: Extend<Self::Item>>(self, collection: &mut E) -> &mut E
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91804 (Make some `Clone` impls `const`)
 - rust-lang#92541 (Mention intent of `From` trait in its docs)
 - rust-lang#93057 (Add Iterator::collect_into)
 - rust-lang#94739 (Suggest `if let`/`let_else` for refutable pat in `let`)
 - rust-lang#94754 (Warn users about `||` in let chain expressions)
 - rust-lang#94763 (Add documentation about lifetimes to thread::scope.)
 - rust-lang#94768 (Ignore `close_read_wakes_up` test on SGX platform)
 - rust-lang#94772 (Add miri to the well known conditional compilation names and values)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d5c05fc into rust-lang:master Mar 10, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 10, 2022
@cormacrelf
Copy link
Contributor

Awesome, thanks everyone, especially @frengor, IIRC first contribution and did a great job. 🦀

@frengor
Copy link
Contributor Author

frengor commented Mar 11, 2022

Awesome, thanks everyone, especially @frengor, IIRC first contribution and did a great job. 🦀

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants