Add remove_item to Vecs #38143

madseagames · 2016-12-03T17:07:45Z

Alright, so if I'm doing anything wrong please let me know. This is my first time contributing to an open source project, and I am very excited by this opportunity.

I would like to propose an addition to the standard libs. I would like to add the method remove_item to vecs. Why?

Simplicity - removing an item from a vec is overly complicated. Consider the following code:

//remove something from a vec.
fn main() {
    //create a vec
    let mut vec = vec![1, 2, 3];
    
    //we want to remove 1 from the vec, but how?
    
    //Step 1: find the position of 1 in the vec
    let pos = vec.iter().position(|x| *x == 1);
    
    //Step 2: Unwrap the position
    let pos_unwrapped = match pos {
        Some(x) => x,
        None => panic!("Coundn't find 1 in vec.")
    };
    
    //Step 3: remove the item.
    vec.remove(pos_unwrapped);
}

A new user of rust needs to know about closures, unwrapping values, iterators and two methods on vecs to simply remove an item. When I was new to rust this caused me many problems.

rust-highfive · 2016-12-03T17:07:59Z

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.

bluss · 2016-12-03T20:24:22Z

src/libcollections/vec.rs

@@ -1296,6 +1296,17 @@ impl<T: PartialEq> Vec<T> {
    pub fn dedup(&mut self) {
        self.dedup_by(|a, b| a == b)
    }
+
+    #[stable(feature = "rust1", since = "1.0.0")]


Hi! This would be introduced as an unstable feature if introduced. This should be an unstable tag, and you specify a new feature name that you invent for feature=. See dedup_by_key in the same file for an example.

Ok, will do. I wasn't entirely sure what to put for feature :P

serprex · 2016-12-03T22:59:52Z

src/libcollections/vec.rs

+            None => return None,
+        };
+
+        self.remove(pos);


Perhaps return type should be Option<(usize, T)>, returning Some((pos, self.remove(pos)))

Why would you return both pos and self.remove(pos)? Would they not be the same thing, an index to the item to be removed?

self.remove(pos) returns the element

Ahh, ok. I was under the impression that remove returned the index to the item removed. Option<(usize, T)> seems like a good idea to me.

steveklabnik · 2016-12-04T04:01:27Z

src/libcollections/vec.rs

@@ -1296,6 +1296,17 @@ impl<T: PartialEq> Vec<T> {
    pub fn dedup(&mut self) {
        self.dedup_by(|a, b| a == b)
    }
+


This line has trailing whitespace which will need to be removed, that's why travis failed

steveklabnik · 2016-12-04T04:01:50Z

/cc @rust-lang/libs

frewsxcv · 2016-12-04T04:36:09Z

src/libcollections/vec.rs

@@ -1296,6 +1296,17 @@ impl<T: PartialEq> Vec<T> {
    pub fn dedup(&mut self) {
        self.dedup_by(|a, b| a == b)
    }
+
+    #[unstable(feature = "remove_from", reason = "recently added", issue = "38143")]


Should this feature be called "remove_item"?

Or maybe even "vec_remove_item"

It should be, I'll call it vec_remove_item

sfackler · 2016-12-04T18:05:57Z

In what context would a user want to know the index from which the value was removed? Similar methods in Java and Python do not return the index AFAIKT.

madseagames · 2016-12-06T23:46:25Z

@sfackler I've been trying to come up with a use case and I've found none. Perhaps Option(T) would be the best way to go?

sfackler · 2016-12-07T17:59:03Z

That's what I would vote for, yeah. It might also be helpful to make the item argument generic over Borrow<T>, which is what we've done for things like binary_search: #37761

aturon · 2016-12-08T01:40:42Z

I'm tentatively in favor of this API, with a couple of changes:

Renaming to simply remove, to bring into alignment with other collections (e.g. HashMap)
Changing the return type to Option<T> as previously suggested.

sfackler · 2016-12-08T02:08:19Z

remove already exists for removal by index I believe.

aturon · 2016-12-09T01:56:59Z

@sfackler Oh whoops, of course. In that case, remove_item is a pretty reasonable name.

madseagames · 2016-12-09T18:26:14Z

Changed return value to Option<T> T being the item returned by remove(pos).

steveklabnik · 2017-01-03T16:45:15Z

I've restarted Travis as it's not clear to me that the failure was legit.

sfackler · 2017-01-17T21:10:34Z

@rfcbot fcp merge

alexcrichton · 2017-01-19T01:11:33Z

@rfcbot fcp merge

rfcbot · 2017-01-19T01:17:53Z

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

brson · 2017-01-20T22:34:41Z

It seems almost certain if we add a function to remove a value by comparison to another concrete value, that we will eventually also be asked to add remove_by, taking a closure. Then we will be asked to add HashMap::remove_by. Does that sound ok?

alexcrichton · 2017-01-20T23:00:28Z

@brson seems reasonable to me, yes.

cristicbz · 2017-01-23T10:35:15Z

My only concern here is to do with remove vs swap_remove. The only times I've ever wanted to do this with a Vec has been with swap_remove (when I want to preserve ordering, it generally ends up involving something more general, better served by retain). Will we find ourselves adding swap_remove_item and swap_remove_by?

aturon · 2017-01-23T21:40:40Z

@cristicbz That's possible, though we should keep in mind that these are all conveniences (and so we don't have to offer every possible combination). That said, in my experience there's relatively little cost in adding something that follows a well-known convention, because you immediately know what it does from the name.

clarfonthey · 2017-01-29T18:00:33Z

src/libcollections/vec.rs

@@ -1296,6 +1296,16 @@ impl<T: PartialEq> Vec<T> {
    pub fn dedup(&mut self) {
        self.dedup_by(|a, b| a == b)
    }
+
+    #[unstable(feature = "vec_remove_item", reason = "recently added", issue = "38143")]


This should have a doc comment clarifying that it removes the first item found.

clarfonthey · 2017-01-29T18:02:58Z

I think that it'd be best to also implement this method for VecDeque for consistency.
Should there be a remove_last_item which uses rposition instead?
I'm personally a fan of find_remove, but we can still go with remove_item.

aturon · 2017-01-31T16:28:27Z

@clarcharr

I think that it'd be best to also implement this method for VecDeque for consistency.

Agreed.

Should there be a remove_last_item which uses rposition instead?

Unclear to me. This is one problem with convenience methods: it's not clear where to draw the line. I'm not opposed, though.

I'm personally a fan of find_remove, but we can still go with remove_item.

I think remove_item is better because the primary action here is removing -- and you're likely to be browsing for remove rather than find if you're looking for this API.

aturon · 2017-01-31T16:28:34Z

Ping @brson

alexcrichton · 2017-02-04T21:54:24Z

@madseagames in preparation for merging, could you be sure to document this method as well?

We also tend to not require full consensus before merging new unstable APIs, so if you want to ping me when updated I'll r+!

alexcrichton · 2017-02-07T19:09:33Z

@madseagames oh also can you file a tracking issue and update the reference? If you cc me on the issue I'll be sure to tag it appropriately

bors · 2017-02-24T02:17:06Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-24T04:30:49Z

@bors: retry * network error

…

On Thu, Feb 23, 2017 at 8:17 PM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2079> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95EtfRjrdMOPKbq-WE_dyBeYhl-Znks5rfj2kgaJpZM4LDW6p> .

bors · 2017-02-24T07:22:13Z

⌛ Testing commit e86846d with merge 0bea301...

bors · 2017-02-24T07:30:07Z

💔 Test failed - status-travis

alexcrichton · 2017-02-25T06:40:32Z

@bors: retry

network error

eddyb · 2017-02-25T09:54:51Z

@bors r- Doc test needs feature-gate too.

madseagames · 2017-03-05T20:15:12Z

@alexcrichton Whats going on with Travis? Anything I need to change? Also, when will we be seeing remove_item in the rust std libs? Will it be part of the nightly or beta?

eddyb · 2017-03-05T20:40:53Z

@madseagames The example in the documentation needs the feature gate too.

alexcrichton · 2017-03-06T17:33:30Z

@madseagames oh the example here needs the same feature name as the method itself (as evidenced by the Travis error)

alexcrichton · 2017-03-07T15:09:33Z

Thanks @madseagames! Could you also squash the commits down into one as well?

eddyb · 2017-03-07T17:11:18Z

src/libcollections/vec.rs

@@ -1165,6 +1165,15 @@ impl<T> Vec<T> {
        }
        other
    }
+
+    pub fn remove_item<T: PartialEq>(&mut self, item: T) {


What's this?

I'm sorry, I'm having some issues with github at the moment, Im working on it, I'm an absolute newbie to github and I'm trying to fix things

I recommend trying out git gui, especially in amend mode, and then force-pushing.

eddyb · 2017-03-07T17:37:35Z

@madseagames You should still squash (git rebase -i c80c31a502c838f9ec06f1003d7c61cf9de9d551 then follow the instructions - you can use something like upstream/master instead of that commit hash but I'm not sure if your https://github.com/rust-lang/rust remote is called upstream).
Also, I think you still have two copies of the definition (again, git gui makes it super easy to fix that).

madseagames · 2017-03-07T17:38:46Z

@eddyb So far I've been doing everything through the github website, is there a way to squash through the site? Or do I need to download the command line tool?

eddyb · 2017-03-07T17:39:57Z

@madseagames Ohhhh that explains things. Well, I'm surprised you got this far, the web UI is unusable for anything more than tiny changes and a single commit. I think I can fix it for you though.

madseagames · 2017-03-07T17:41:22Z

@eddyb How would you go about doing that? Now that you mention it I do have two definitions...

clarfonthey · 2017-03-07T17:46:08Z

@madseagames if you use Windows or OS X, you should be able to download GitHub's git UI that should let you squash pull requests.

eddyb · 2017-03-07T17:47:08Z

Uhm, this is very surprising.

eddyb · 2017-03-07T17:47:37Z

@madseagames I tried to use a new GitHub feature but it emptied&closed the pull request instead.

EDIT: Aaaaand let it be known I suck at git, I used the wrong source branch, see #40325.

madseagames · 2017-03-07T17:58:15Z

Haha, glad to know I'm not the only one that sucks at git :P also @clarcharr I run Linux. I tried to learn git once but figured that taking so much time to learn such a complex tool wasn't worth it.

eddyb · 2017-03-07T17:59:32Z

@madseagames Oh, all of this could've been avoided, when you said "download the command-line tool" I thought you were on Windows where it's actually some work 😆 (if you want to learn, again, git gui is your friend - had I used it to push here I wouldn't have pushed the wrong thing to your branch).

madseagames · 2017-03-07T18:01:52Z

@eddyb so... Are things fixed? I'm still not entirely sure. I removed the second definition, did you squash the commits?

eddyb · 2017-03-07T18:04:39Z

@madseagames This PR is broken permanently AFAICT, had to open #40325 (which has 1 commit).

madseagames · 2017-03-07T18:05:46Z

@eddyb what do I need to do at my end?

eddyb · 2017-03-07T18:08:09Z

@madseagames Hopefully nothing, if #40325 merges.

@madseagames

Added remove_from to vec.rs (rust-lang#38143) Turns out that if you push to someone's PR branch and cause the PR to close, you lose delegation 😞. @madseagames I'm really sorry about that 😭

@madseagames

Added remove_from to vec.rs (rust-lang#38143) Turns out that if you push to someone's PR branch and cause the PR to close, you lose delegation 😞. @madseagames I'm really sorry about that 😭

@madseagames

Added remove_from to vec.rs (rust-lang#38143) Turns out that if you push to someone's PR branch and cause the PR to close, you lose delegation 😞. @madseagames I'm really sorry about that 😭

rust-highfive assigned sfackler Dec 3, 2016

bluss reviewed Dec 3, 2016

View reviewed changes

serprex reviewed Dec 3, 2016

View reviewed changes

steveklabnik reviewed Dec 4, 2016

View reviewed changes

frewsxcv reviewed Dec 4, 2016

View reviewed changes

madseagames changed the title ~~Add remove_from to Vecs~~ Add remove_item to Vecs Dec 4, 2016

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2017

clarfonthey reviewed Jan 29, 2017

View reviewed changes

eddyb reviewed Mar 7, 2017

View reviewed changes

eddyb closed this Mar 7, 2017

eddyb force-pushed the master branch from c364863 to e1cb9ba Compare March 7, 2017 17:46

Add remove_item to Vecs #38143

Add remove_item to Vecs #38143

Conversation

madseagames commented Dec 3, 2016 • edited Loading

rust-highfive commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Dec 4, 2016

madseagames commented Dec 6, 2016

sfackler commented Dec 7, 2016

aturon commented Dec 8, 2016

sfackler commented Dec 8, 2016

aturon commented Dec 9, 2016

madseagames commented Dec 9, 2016 • edited Loading

steveklabnik commented Jan 3, 2017

sfackler commented Jan 17, 2017

alexcrichton commented Jan 19, 2017

rfcbot commented Jan 19, 2017 • edited by brson Loading

brson commented Jan 20, 2017

alexcrichton commented Jan 20, 2017

cristicbz commented Jan 23, 2017

aturon commented Jan 23, 2017

clarfonthey Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

clarfonthey commented Jan 29, 2017 • edited Loading

aturon commented Jan 31, 2017

aturon commented Jan 31, 2017

alexcrichton commented Feb 4, 2017

alexcrichton commented Feb 7, 2017

bors commented Feb 24, 2017

alexcrichton commented Feb 24, 2017 via email

bors commented Feb 24, 2017

bors commented Feb 24, 2017

alexcrichton commented Feb 25, 2017

eddyb commented Feb 25, 2017

madseagames commented Mar 5, 2017

eddyb commented Mar 5, 2017

alexcrichton commented Mar 6, 2017

alexcrichton commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

eddyb commented Mar 7, 2017 • edited Loading

madseagames commented Mar 7, 2017

eddyb commented Mar 7, 2017

madseagames commented Mar 7, 2017 • edited Loading

clarfonthey commented Mar 7, 2017

eddyb commented Mar 7, 2017

eddyb commented Mar 7, 2017 • edited Loading

madseagames commented Mar 7, 2017

eddyb commented Mar 7, 2017 • edited Loading

madseagames commented Mar 7, 2017

eddyb commented Mar 7, 2017

madseagames commented Mar 7, 2017

eddyb commented Mar 7, 2017

madseagames commented Dec 3, 2016 •

edited

Loading

madseagames commented Dec 9, 2016 •

edited

Loading

rfcbot commented Jan 19, 2017 •

edited by brson

Loading

clarfonthey Jan 29, 2017 •

edited

Loading

clarfonthey commented Jan 29, 2017 •

edited

Loading

eddyb Mar 7, 2017 •

edited

Loading

eddyb commented Mar 7, 2017 •

edited

Loading

madseagames commented Mar 7, 2017 •

edited

Loading

eddyb commented Mar 7, 2017 •

edited

Loading

eddyb commented Mar 7, 2017 •

edited

Loading