bpo-32792: Preserve mapping order in ChainMap()#5586
bpo-32792: Preserve mapping order in ChainMap()#5586rhettinger merged 4 commits intopython:masterfrom
Conversation
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM, but let ask @ned-deily for backporting.
|
|
||
| def test_dict_coercion(self): | ||
| d = ChainMap(dict(a=1, b=2), dict(b=20, c=30)) | ||
| self.assertEqual(dict(d), dict(a=1, b=2, c=30)) |
There was a problem hiding this comment.
Maybe add tests for OrderedDict(d) and OrderedDict(d.items)?
|
I'm fine with merging this in time for 3.7.0b2. While changing the behavior to preserve mapping order would have been the right thing to do for 3.6.0, I question whether it is appropriate to make such a behavior change in a 3.6.x maintenance release. As best I read it, the 3.6 ChainMap docs make no commitment one-way or the other about iteration order. And in previous similar discussions about ordering guarantees, my take on Guido's intent is that, at this point, the status quo wins in 3.6.x (for example, https://mail.python.org/pipermail/python-dev/2017-December/151321.html or bpo-32690). So let's merge for master and 3.7. Thanks! |
which is faster and gives a more useful ordering.
| def __iter__(self): | ||
| return iter(set().union(*self.maps)) | ||
| d = {} | ||
| for mapping in reversed(self.maps): |
There was a problem hiding this comment.
Sorry, I was wrong. reversed() is not needed here.
There was a problem hiding this comment.
I think you were right and that the deepest mappings should determine the order while the shallowest mappings determine the value.
This will be the most useful behavior in chains like command_line_args -> enviroment_variables -> default_values. The default values can have the standard ordering while the overriding environment variables and command line arguments can be arbitrarily ordered.
Also, using reversed() is consistent with the notion of a ChainMap being like a base dictionary will several successive updates where the base layer determines order and the new layers either add new items or update values without changing their order.
There was a problem hiding this comment.
Currently list(ChainMap({1: int}, {1.0: float})) returns [1]. With reversed() it will return [1.0].
There was a problem hiding this comment.
I think the [1.0] is correct. It also matches what OrderedDict does when it encounters items with equal keys and different values:
>>> OrderedDict([(1.0, float), (1, int)]).items()
odict_items([(1.0, <class 'int'>)])
There was a problem hiding this comment.
FWIW, except in the odd case of 1 and 1.0, the current behavior is unordered. As an example, list(ChainMap({'one': int}, {'two': float}) sometimes returns ['one', 'two'] and sometimes returns ['two', 'one'].
There was a problem hiding this comment.
I understand your arguments. Such interpretation makes sense to me. But there are two changes in this PR:
-
The order was undetermined, now it will be determined. This doesn't break any code, because you could get the new order with old behavior.
-
In case of equal keys the behavior is changed. The first wined before, the last key will win with this change. This is subtle behavior change, but it is a change.
It would be better to discuss this on the tracker. This is not some implementation detail, but a design question.
There was a problem hiding this comment.
To be clear, this behavior change LGTM, but I just want to make clear that there is a behavior change.
|
Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
(cherry picked from commit 3793f95) Co-authored-by: Raymond Hettinger <[email protected]>
|
GH-5617 is a backport of this pull request to the 3.7 branch. |
(cherry picked from commit 3793f95) Co-authored-by: Raymond Hettinger <[email protected]>
https://bugs.python.org/issue32792