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.

Sign up for GitHub

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

Jump to bottom

Add take as alias for remove in collection methods #79943

Closed
wants to merge 3 commits into from
Closed

Add take as alias for remove in collection methods #79943

wants to merge 3 commits into from

Conversation

mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Dec 11, 2020

This partially alleviates the issues described in RFC 3034 [0] by adding
a doc alias to take and take_xxx for remove and remove_xxx.

[0]: rust-lang/rfcs#3034

r? @sfackler

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2020
@scottmcm
Copy link
Member

Should this happen for swap_remove too?

(Actually, I'm surprised to not see any Vec changes in here -- any chance you missed a file in the commit?)

@camelid camelid added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 15, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@bors
Copy link
Contributor

bors commented Feb 3, 2021

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

@mqudsi
Add take as alias for remove in collection methods
c383e01 
This partially alleviates the issues described in RFC 3034 [0] by adding
a doc alias to `take` and `take_xxx` for `remove` and `remove_xxx`.

[0]: rust-lang/rfcs#3034

r? @sfackler
@mqudsi
Add take() alias to Vec::remove()
c115c3c
@mqudsi
Add swap_take() alias to swap_remove() and co
f833593
@mqudsi
Copy link
Contributor Author

mqudsi commented Feb 5, 2021

@scottmcm Yeah, it does look like Vec was left out somehow. I'm actually not sure about swap_remove, mainly because the name is already suboptimal (does it swap or does it remove? Oh, it removes by swapping, but not with anything I provide!), so you'd almost certainly have to know it beforehand (vs applying a pattern you know from Option and friends) but given that this is going to be just a doc/linter hint and not an actual name change, there's not too much to lose by adding swap_take as an alias to swap_remove.

I rebased and added the missing aliases, if you don't mind taking a look.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2021
@Dylan-DPC-zz
Copy link

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned sfackler Mar 13, 2021
@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2021
@Dylan-DPC-zz
Copy link

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned m-ou-se Mar 31, 2021
@Amanieu Amanieu requested a review from crlf0710 April 14, 2021 19:50
@Amanieu Amanieu assigned m-ou-se and unassigned Amanieu Apr 14, 2021
@Amanieu Amanieu removed the request for review from crlf0710 April 14, 2021 19:51
@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

Thanks for your PR and sorry for the late reply. This has been sitting still without review because it's not quite clear what exactly we want to use doc(alias) for.

See #81989 (comment).

For the specific aliases added in this PR, I'm wondering how useful they will be. These aliases are not displayed anywhere in the documentation, and only used when using the search functionality. I'd expect someone who is looking for methods on e.g. HashMap to be looking through the HashMap page, instead of entering just take into the search bar.

Anyway, as mentioned in the linked comment, we'd like to first reach some agreement on what kind of aliases we want and don't want before continuing merging PRs like this:

[..] we want some kind of clear guideline/policy that we can all agree on before accepting more aliases. Feel free to open an issue on https://github.com/rust-lang/libs-team or ping us on Zulip if you (anyone) want to work on this.

So, I'm closing this PR for now. Once we have a policy in place and this PR fits within it, feel free to re-open it.

@m-ou-se m-ou-se closed this Apr 21, 2021
@jyn514 jyn514 mentioned this pull request Apr 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.