Document replacement behavior in some collections #27894
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
So we're 100% commiting to this behaviour? (before it was unspecified but agreed upon) |
|
I thought it was committed to. |
|
/cc @rust-lang/libs |
|
Yeah I just one last emergency "THIS IS STUPID" check -- but I think this is the right default, and can be overridden by @apasel422's work if desired. |
| /// If the set did not have a value present, `true` is returned. | ||
| /// | ||
| /// If the set already had a value present, that value is | ||
| /// returned, and the entry is not updated. See the examples below for more. |
There was a problem hiding this comment.
Insert returns a bool, not any value here.
|
So, is this PR still wanted? if so I'll fix whatever's up with Travis. |
|
Still not sure what to do here. @rust-lang/libs ? Anything? |
alexcrichton
added
the
T-libs-api
| /// If the map did not have this key present, `None` is returned. | ||
| /// | ||
| /// If the map did have this key present, that value is returned, and the | ||
| /// entry is not updated. See the examples below for more. |
There was a problem hiding this comment.
By "entry is not updated" you specifically mean the key is not updated; presumably the value is?
|
The libs team discussed this at triage today and the conclusion was that we do indeed want to document the current behavior, and the current behavior is OK. One point brought up though its that it's probably not worth it to include very long examples in 4 separate locations for a pretty niche use case, perhaps the docs could just mention what happens in this case? |
|
I included the examples because I find it easier to actually understand what's going on. Even relatively clear text didn't make it sink in for me, to be honest. But I can see how it might not be worth it as well. |
|
Ah yeah I agree it's probably easier to understand, but it's not really something that everyone needs to be bombarded with when looking at the documentation. (e.g. the set of people reading the docs is far larger than the set of those who need to understand this). Perhaps one collection could have an example, and the others could reference it? |
|
I've updated it to just point at the module-level docs each time. |
|
@bors: r+ ebf61e2e95605868b399210aa907408a6dc1955a |
|
⌛ Testing commit ebf61e2 with merge 9a8a441... |
|
💔 Test failed - auto-win-msvc-32-opt |
|
@bors: retry On Fri, Oct 16, 2015 at 4:28 PM, bors [email protected] wrote:
|
|
⌛ Testing commit ebf61e2 with merge ad7df2e... |
|
💔 Test failed - auto-mac-32-opt |
{BTree,Hash}{Map,Set} will not update their key if it already exists, which
can matter with more complex keys. This behavior is now documented.
Fixes rust-lang#26888
|
@bors: r=alexcrichton fixed the tidy issue, how embarassing :( |
|
📌 Commit e8e3c6f has been approved by |
{BTree,Hash}{Map,Set} will not update their key if it already exists, which
can matter with more complex keys. This behavior is now documented.
Fixes #26888
|
@steveklabnik your quest is over! \o/ You can now retire in honour. |
|
@gankro whew! |
{BTree,Hash}{Map,Set} will not update their key if it already exists, which
can matter with more complex keys. This behavior is now documented.
Fixes #26888