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

Map::Entry methods to recover key and value together #33300

Merged
merged 1 commit into from
Jun 15, 2016

Conversation

seanmonstar
Copy link
Contributor

@seanmonstar
Copy link
Contributor Author

r? @sfackler

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive assigned sfackler and unassigned brson Apr 30, 2016
@sfackler
Copy link
Member

sfackler commented May 1, 2016

Compilation errors: https://travis-ci.org/rust-lang/rust/builds/126956602

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 1, 2016
@seanmonstar
Copy link
Contributor Author

Fixed

@seanmonstar
Copy link
Contributor Author

An alternative that may be nice (and keep the method names consistent) is to impl Into<K> for VacantEntry and impl Into<(K, V)> for OccupiedEntry.

@alexcrichton
Copy link
Member

The libs team discussed this during triage the other day and the conclusion was that this seems like fine functionality to have but we likely want to closely consider the naming.

I would personally propose into_key for VacantEntry and remove_pair for OccupiedEntry as the former conveys the transfer of ownership and the latter mirrors the existing remove function along with the returned pair (although the term "pair" is kinda unfortunate).

@alexcrichton
Copy link
Member

cc @rust-lang/libs

@arielb1
Copy link
Contributor

arielb1 commented May 12, 2016

Maybe Python-style remove_item?

@seanmonstar
Copy link
Contributor Author

I've changed the names to into_key and remove_pair.

@@ -1898,6 +1898,12 @@ impl<'a, K: Ord, V> VacantEntry<'a, K, V> {
&self.key
}

/// Take ownership of the key.
#[unstable(feature = "map_entry_keys", issue = "32281")]
Copy link
Member

Choose a reason for hiding this comment

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

Could these point to a new issue? Pointing to a close issue may unfortunately mean that these never come up for stabilization :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. Probably need a new feature as well, cause map_entry_keys is now stable. So just create a new issue "Tracking for map_entry_recover_keys" or some such?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I can be sure to tag it appropriately and such.

@seanmonstar seanmonstar changed the title Map::Entry::take() method to recover key and value together Map::Entry methods to recover key and value together Jun 15, 2016
@seanmonstar
Copy link
Contributor Author

Added map_entry_recover_keys feature, pointing at #34285

@alexcrichton
Copy link
Member

@bors: r+ 217a964

bors added a commit that referenced this pull request Jun 15, 2016
Map::Entry methods to recover key and value together

See #32281 (comment)
@bors
Copy link
Contributor

bors commented Jun 15, 2016

⌛ Testing commit 217a964 with merge 9b06d2a...

@bors bors merged commit 217a964 into rust-lang:master Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants