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

bpo-32792: Preserve mapping order in ChainMap() #5586

Merged
merged 4 commits into from Feb 11, 2018

Conversation

Projects
None yet
6 participants
@rhettinger
Copy link
Contributor

rhettinger commented Feb 8, 2018

@serhiy-storchaka
Copy link
Member

serhiy-storchaka left a comment

LGTM, but let ask @ned-deily for backporting.

self.assertEqual(list(d.items()), [('a', 1), ('b', 2), ('c', 4),
('d', 6), ('e', 8), ('f', 10),
('g', 11), ('h', 12)])

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))

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Feb 8, 2018

Member

Maybe add tests for OrderedDict(d) and OrderedDict(d.items)?

@ned-deily

This comment has been minimized.

Copy link
Member

ned-deily commented Feb 8, 2018

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!

Adopt Serhiy's suggested alternative algorithm
which is faster and gives a more useful ordering.
@@ -920,7 +920,10 @@ def __len__(self):
return len(set().union(*self.maps)) # reuses stored hash values if possible

def __iter__(self):
return iter(set().union(*self.maps))
d = {}
for mapping in reversed(self.maps):

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Feb 10, 2018

Member

Sorry, I was wrong. reversed() is not needed here.

This comment has been minimized.

Copy link
@rhettinger

rhettinger Feb 10, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Feb 11, 2018

Member

Currently list(ChainMap({1: int}, {1.0: float})) returns [1]. With reversed() it will return [1.0].

This comment has been minimized.

Copy link
@rhettinger

rhettinger Feb 11, 2018

Author Contributor

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'>)])

This comment has been minimized.

Copy link
@rhettinger

rhettinger Feb 11, 2018

Author Contributor

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'].

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Feb 11, 2018

Member

I understand your arguments. Such interpretation makes sense to me. But there are two changes in this PR:

  1. 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.

  2. 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.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Feb 11, 2018

Member

To be clear, this behavior change LGTM, but I just want to make clear that there is a behavior change.

@rhettinger rhettinger merged commit 3793f95 into python:master Feb 11, 2018

4 checks passed

bedevere/issue-number Issue number 32792 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Feb 11, 2018

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

miss-islington added a commit to miss-islington/cpython that referenced this pull request Feb 11, 2018

bpo-32792: Preserve mapping order in ChainMap() (pythonGH-5586)
(cherry picked from commit 3793f95)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Feb 11, 2018

GH-5617 is a backport of this pull request to the 3.7 branch.

@rhettinger rhettinger deleted the rhettinger:chainmap-order branch Feb 11, 2018

rhettinger added a commit that referenced this pull request Feb 11, 2018

bpo-32792: Preserve mapping order in ChainMap() (GH-5586) (#GH-5617)
(cherry picked from commit 3793f95)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.