bpo-32337: Update tutorial and documentation related with dict order. by shangdahao · Pull Request #4973 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation61 Commits11 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| @@ -497,7 +497,8 @@ You can't use lists as keys, since lists can be modified in place using index |
|---|
| assignments, slice assignments, or methods like :meth:`append` and |
| :meth:`extend`. |
| It is best to think of a dictionary as an unordered set of *key: value* pairs, |
| Dictionaries are guaranteed to retain insertion order. |
| It is best to think of a dictionary as an ordered set of *key: value* pairs, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dictionary as an ordered set can be misindestood as ordered by key, intead of ordered by insertion order.
It best to write here, that dict will be enumerated (.keys(), items(), .values()) in order of insertion. And also specify "not ordered by key".
C++ has orderdmap and unorderedmap. Orderedmap orders by key.
Also, in some natural languages, "ordered" and "sorted" are the same :(
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, thanks.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know if other languages are relevant here. If Python docs make a clear explanation of sorted and ordered then things should not be confusing.
| @@ -510,7 +511,7 @@ value associated with that key is forgotten. It is an error to extract a value |
|---|
| using a non-existent key. |
| Performing ``list(d.keys())`` on a dictionary returns a list of all the keys |
| used in the dictionary, in arbitrary order (if you want it sorted, just use |
| used in the dictionary, in insertion order (if you want it sorted, just use |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add example how to "permanently" sort a dict ? like:
xxx = dict(sorted(xxx.items(), key=operator.itemgetter(0)))
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s make sense, thanks.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This is tutorial. We should focus about very important part.
Previously, "unordered" was important for new users to avoid pitfall.
On the other hand, "preserving insertion order" is not so important.
| @@ -1176,6 +1176,48 @@ def iter_and_mutate(): |
|---|
| self.assertRaises(RuntimeError, iter_and_mutate) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to test order for dict comprehensions ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks!
| @@ -497,7 +497,9 @@ You can't use lists as keys, since lists can be modified in place using index |
|---|
| assignments, slice assignments, or methods like :meth:`append` and |
| :meth:`extend`. |
| It is best to think of a dictionary as an unordered set of *key: value* pairs, |
| Dictionaries are guaranteed to retain insertion order, not ordered by key. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove unordered, and don't mention about ordering here.
While dict preserves insertion order, it's not so important in "What is dict?".
For example, dict.__eq__ ignores order.
Done. Thanks methane for your explanation.
I feel NEWS entry is too short.
And did you saw this comment ?
Oh sorry, I did't saw it.
That solution is great, and I am going to remove the tests in this PR.
And I am wondering is there necessary to keep the "test_copy_ordering" and 'test_comprehension_ordering' here? Since there have no tests for them.
bpo-32337: Add tests and update doc for dict order. bpo-32337: Update doc for dict order.
bpo-32337: Update doc for dict order. bpo-32337: Update tutorial of dict
bpo-32337: Update tutorial of dict bpo-32337: Update dict tutorial for preserving insertion order behavior
I'm OK for document body, but I think someone can write more good commit message and NEWS entry. (Sorry, I'm not good English writer)
This PR updates only tutorial. But I think other commits will be made before 3.7 final can
share same NEWS entry. (Lot's of redundant NEWS entry make chengelog less usable)
Since we are editing this part, I have a question why do we use list(dict.keys()) and sorted(dict.keys())? Isn't list(dict) or sorted(dict) enough? I have seen many codes using this redundant keys call, even in Python2 testing membership.
| @@ -1788,3 +1788,4 @@ Jelle Zijlstra |
|---|
| Gennadiy Zlobin |
| Doug Zongker |
| Peter Åstrand |
| Shang Hui |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this line with other S or H names? (It’s loosely sorted by last name, for names that have a concept of last name)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will do it.
Thanks zhangyangyu and merwok, I have made a update as your suggestions.
It's just my question. I am not sure which is actually wanted. But at least if you change it, the footprint link is not needed either.
I think that people may use d.keys() for membership or iteration because they may not be fully comfortable with the fact that iteration/membership on dicts uses keys, not items.
I don’t know if the keys method pre-dates the iteration protocol, but given that keys was never deprecated, to me it’s an indication that it’s fine if people use it, even if it’s technically redundant.
Here in the tutorial, I would go for not changing it.
I think that people may use d.keys() for membership or iteration because they may not be fully comfortable with the fact that iteration/membership on dicts uses keys, not items.
But tutorial should teach preferred way, not redundant way?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| @@ -224,7 +224,7 @@ For example:: |
|---|
| .. class:: Counter([iterable-or-mapping]) |
| A :class:`Counter` is a :class:`dict` subclass for counting hashable objects. |
| It is an unordered collection where elements are stored as dictionary keys |
| It is a collection where elements are stored as dictionary keys |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found more occurrences of «arbitrary order» and usage of sorted to make examples deterministic in the rest of the file. I think careful thought should be given to the guarantees given in this file: it may be easy to say that elements returns in insertion order, but most_common may need to stay under-defined.
If you could get the attention of @rhettinger on the bug tracker to see what’s the best thing to do here, or possibly to not touch this file in this PR, that would be best.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you great review, but I am not very understand your meaning about:
"but most_common may need to stary under-defined.".
Could you explain it a little more?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo!
The basic idea is that if we guarantee that all these methods return things in insertion order now, we may preclude future implementation changes or optimizations. Each method needs to be checked to see if it’s useful to change the doc that says «unordered» (if it’s accurate now) or not (if we don’t want to guarantee it).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merwok How about creating another pull request about it?
This pull request is too long discussed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if a file is updated, it should be updated exhaustively, not add more ambiguity than was before. But I don’t want to block the updates to the tutorial, so feel free to merge when you are satisfied.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
And if you don't make the requested changes, you will be poked with soft cushions!
I have made the requested changes; please review again
Document somewhere that two dicts with same contents, but different order of key-value pairs are equal.
i.e. assert {1:2, 3:4} == {3:4, 1:2}.
It is not obvious, and I needed to check that before use.
@socketpair Dict equality is already documented to not rely on order.
Let's merge this as-is. We discussed long enough.
We can improve document later.
Thanks @shangdahao for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
(cherry picked from commit dfbbbf1)
Co-authored-by: hui shang shangdahao@gmail.com
miss-islington added a commit that referenced this pull request
(cherry picked from commit dfbbbf1)
Co-authored-by: hui shang shangdahao@gmail.com
Labels
Documentation in the Doc dir