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

Clarify Extend behaviour wrt existing keys #38636

Merged
merged 2 commits into from
Jan 14, 2017
Merged

Conversation

shahn
Copy link
Contributor

@shahn shahn commented Dec 27, 2016

This seems to be consistent with all the Extend implementations I found, and isn't documented anywhere else afaik.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@shahn
Copy link
Contributor Author

shahn commented Dec 27, 2016

r? @steveklabnik

@rust-highfive rust-highfive assigned steveklabnik and unassigned aturon Dec 27, 2016
@steveklabnik
Copy link
Member

@rust-lang/libs is this something we want to guarantee here?

@sfackler
Copy link
Member

sfackler commented Jan 3, 2017

I would probably move this documentation to the implementations themselves - this wording may not make sense for a multimap or multiset, for example.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

@sfackler one might argue the phrase "the entry is updated" is open-ended enough to cover multimaps/multisets. It is certainly more appropos than saying "the entry is replaced", at least to me.

(I assume that the critical detail the author desires is to convey that such entries are not ignored during the traversal. Is that open-ended enough to commit to?)

@shahn
Copy link
Contributor Author

shahn commented Jan 3, 2017

Yes indeed, that's what this is about. How about the following wording: the entry is updated or newly added if multiple elements with the same key are permitted."?

@shahn
Copy link
Contributor Author

shahn commented Jan 5, 2017

I have pondered a bit on the wording and provided an updated wording, is that better? I would really like to avoid making the Extend behaviour implementation documented, because that's a lot of places to look and a chance for confusing behaviour/inconsistent docs to creep in.

@steveklabnik
Copy link
Member

I like this wording, I think. @sfackler @pnkfelix ?

@sfackler
Copy link
Member

sfackler commented Jan 5, 2017

👍

@steveklabnik
Copy link
Member

@bors: r+ rollup

thanks!

@bors
Copy link
Contributor

bors commented Jan 11, 2017

📌 Commit 74b2587 has been approved by steveklabnik

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 13, 2017
Clarify Extend behaviour wrt existing keys

This seems to be consistent with all the Extend implementations I found, and isn't documented anywhere else afaik.
bors added a commit that referenced this pull request Jan 13, 2017
Rollup of 10 pull requests

- Successful merges: #38362, #38636, #38877, #38946, #38965, #38986, #38994, #38995, #39024, #39027
- Failed merges:
@bors bors merged commit 74b2587 into rust-lang:master Jan 14, 2017
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.

7 participants