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

[RFC] Entry API v3: replace Entry::get with Entry::default and Entry::default_with #22930

Merged
merged 3 commits into from
Mar 27, 2015

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Mar 1, 2015

RFC pending, but this is the patch that does it.

Totally untested. Likely needs some removed imports. std::collections docs should also be updated to provide better examples.

Closes #23508

@rust-highfive
Copy link
Collaborator

r? @brson

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

@Gankra Gankra force-pushed the entry_3 branch 3 times, most recently from d7782ad to 009f861 Compare March 1, 2015 15:04
reason = "matches entry v3 specification, waiting for dust to settle")]
/// Ensures a value is in the entry by inserting the default if empty, and returns
/// a mutable reference to the value in the entry.
pub fn default(self. default: V) -> &'a mut V {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/self./self,

@Gankra
Copy link
Contributor Author

Gankra commented Mar 2, 2015

RFC posted: rust-lang/rfcs#921

All imports/typos/missed-things fixed.

@bors
Copy link
Contributor

bors commented Mar 7, 2015

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

@aturon
Copy link
Member

aturon commented Mar 19, 2015

Note: the RFC has now been merged (with updated names)

@Gankra Gankra force-pushed the entry_3 branch 2 times, most recently from c9b5258 to 0c4a120 Compare March 20, 2015 17:43
@Gankra
Copy link
Contributor Author

Gankra commented Mar 20, 2015

Rebased with updated names. r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned brson Mar 20, 2015
@alexcrichton
Copy link
Member

(added Closes #23508 to the description)

I'd also be fine insta-stabilizing these methods as they've been around for quite some time and we're just tweaking the naming here basically.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 20, 2015

I'm not comfortable stabilizing these quite yet. We could go one step further and just make these methods on the Map itself, for instance.

I also see no particular urgency to stabilize them; they are pure convenience (though very convenient indeed).

@Gankra
Copy link
Contributor Author

Gankra commented Mar 20, 2015

Oh whoops I forgot to actually, you know, test the rebase/fix. Tidy error fixed; building now.

@aturon
Copy link
Member

aturon commented Mar 20, 2015

This is really, really nice! You've done great work with Entry, @gankro.

I also agree that there's no rush to stabilize these new methods, which are more than a rename.

@aturon
Copy link
Member

aturon commented Mar 20, 2015

r=me once tests are looking good locally

@Gankra
Copy link
Contributor Author

Gankra commented Mar 20, 2015

looks good locally (had to make one update)

@@ -1126,6 +1126,8 @@ impl<'a, K, V> DoubleEndedIterator for RangeMut<'a, K, V> {
impl<'a, K: Ord, V> Entry<'a, K, V> {
#[unstable(feature = "collections",
reason = "matches collection reform v2 specification, waiting for dust to settle")]
#[deprecated(since = "1.0",
reason = "replaced with more ergonomic `default` and `default_with`")]
Copy link
Contributor

Choose a reason for hiding this comment

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

or_insert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@aturon
Copy link
Member

aturon commented Mar 23, 2015

(r=me after nits)

@bors
Copy link
Contributor

bors commented Mar 23, 2015

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

@aturon
Copy link
Member

aturon commented Mar 23, 2015

Given the proximity to beta, I'm thinking we should probably go ahead and mark these #[stable] within this PR as well. I know that this is more than a straight renaming, but I still think it's a relatively small change to the API with clear benefits, and stabilizing it now will enable a lot of ecosystem code to continue using it at beta.

Worst case, if we do find a problem, we can revise before 1.0.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 24, 2015

I'd really like to let this sit if possible. Although empirically it seems like we've played very fast and loose with stable vs unstable so far. Is it possible to stabilize things in the beta -> stable transition?

There's still (in my mind) open questions about whether this functionality should just be on the map directly. Although I suppose we can deprecate this in favour of that quite easily.

@aturon
Copy link
Member

aturon commented Mar 24, 2015

@gankro I'm happy to defer to your judgment here.

In terms of ongoing stabilization: we will be adding unstable APIs, and stabilizing existing ones, continuously. It is likely possible to stabilize a few APIs during the 1.0 beta and have those available at 1.0, but it's not entirely clear yet.

In terms of "fast and loose", I'm not sure exactly what you're referring to -- perhaps just that we've been willing to change #[stable] APIs in the past? This is entirely due to being in a pre-1.0 state, where it's been essential to use the markers to track progress in cleaning up APIs and signal preliminary confidence in them, while still retaining the freedom to change until the actual release. These kinds of changes have all but disappeared since the alpha release, though.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 24, 2015

Easy to stabilize later; let's land this unstable for now. Can review in coming weeks, perhaps with removal of deprecated API.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 24, 2015

@bors r=aturon

@bors
Copy link
Contributor

bors commented Mar 24, 2015

📌 Commit d3d11b2 has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 24, 2015

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

@Gankra
Copy link
Contributor Author

Gankra commented Mar 27, 2015

Ugh what another rebase?

@Gankra
Copy link
Contributor Author

Gankra commented Mar 27, 2015

@bors r=aturon

@bors
Copy link
Contributor

bors commented Mar 27, 2015

📌 Commit 1132198 has been approved by aturon

bors added a commit that referenced this pull request Mar 27, 2015
RFC pending, but this is the patch that does it.

Totally untested. Likely needs some removed imports. std::collections docs should also be updated to provide better examples.

Closes #23508
@bors
Copy link
Contributor

bors commented Mar 27, 2015

⌛ Testing commit 1132198 with merge 64a4e01...

@bors
Copy link
Contributor

bors commented Mar 27, 2015

💔 Test failed - auto-linux-64-x-android-t

@Gankra
Copy link
Contributor Author

Gankra commented Mar 27, 2015

@bors r=aturon

@bors
Copy link
Contributor

bors commented Mar 27, 2015

📌 Commit 1b98f6d has been approved by aturon

@bors
Copy link
Contributor

bors commented Mar 27, 2015

⌛ Testing commit 1b98f6d with merge 242ed0b...

bors added a commit that referenced this pull request Mar 27, 2015
RFC pending, but this is the patch that does it.

Totally untested. Likely needs some removed imports. std::collections docs should also be updated to provide better examples.

Closes #23508
@bors bors merged commit 1b98f6d into rust-lang:master Mar 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for Entry API V3
8 participants