python / cpython

The Python programming language
https://www.python.org
Other
63.19k stars 30.26k forks source link

Dictionary union. (PEP 584) #80325

Closed brandtbucher closed 4 years ago

brandtbucher commented 5 years ago
BPO 36144
Nosy @gvanrossum, @rhettinger, @mdickinson, @scoder, @serhiy-storchaka, @zooba, @MojoVampire, @aaronchall, @3lnc, @tirkarthi, @brandtbucher, @curtisbucher, @chaburkland, @justjais
PRs
  • python/cpython#12088
  • python/cpython#18659
  • python/cpython#18729
  • python/cpython#18814
  • python/cpython#18832
  • python/cpython#18911
  • python/cpython#18931
  • python/cpython#18967
  • python/cpython#19106
  • python/cpython#19127
  • python/cpython#19221
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = created_at = labels = ['interpreter-core', 'type-feature', '3.9'] title = 'Dictionary union. (PEP 584)' updated_at = user = 'https://github.com/brandtbucher' ``` bugs.python.org fields: ```python activity = actor = 'Raps Uk' assignee = 'none' closed = True closed_date = closer = 'brandtbucher' components = ['Interpreter Core'] creation = creator = 'brandtbucher' dependencies = [] files = [] hgrepos = [] issue_num = 36144 keywords = ['patch'] message_count = 66.0 messages = ['336798', '336803', '336808', '336810', '336811', '336812', '336816', '336820', '336847', '336848', '336849', '336854', '337094', '337107', '337266', '337267', '358305', '362619', '362620', '362621', '362626', '362645', '362660', '362663', '362666', '362726', '362729', '362740', '362757', '362759', '362761', '362774', '362779', '362810', '362813', '362816', '362817', '362819', '362821', '362823', '362827', '362828', '362831', '362835', '362852', '362855', '363526', '363527', '363528', '363613', '363625', '363889', '363953', '364106', '364107', '364195', '364196', '364887', '364900', '364969', '364992', '365245', '365333', '371520', '371549', '373205'] nosy_count = 15.0 nosy_names = ['gvanrossum', 'rhettinger', 'mark.dickinson', 'scoder', 'serhiy.storchaka', 'steve.dower', 'josh.r', 'Aaron Hall', 'slam', 'xtreak', 'brandtbucher', 'curtisbucher', 'chaburkland', 'justjais', 'Raps Uk'] pr_nums = ['12088', '18659', '18729', '18814', '18832', '18911', '18931', '18967', '19106', '19127', '19221'] priority = 'normal' resolution = 'fixed' stage = 'patch review' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue36144' versions = ['Python 3.9'] ```

    brandtbucher commented 4 years ago

    bpo-39857 just reminded me that we should update os._Environ as well (the type of os.environ and os.environb).

    I have another first-timer who will probably want to take it.

    vstinner commented 4 years ago

    Once this issue will be done, would you mind to update PEP-584? Currently, it only says "This PEP proposes adding merge (|) and update (|=) operators to the built-in dict class." It doesn't mention that other types like OrderedDict, MappingProxy or ChainMap are updated as well.

    brandtbucher commented 4 years ago

    Yes, I can add a section explaining that after the PEP was accepted, we decided to add the operators to several non-dict mappings as well. I'm also going to add some explanation as to why Mapping/MutableMapping didn't grow them, too.

    gvanrossum commented 4 years ago

    New changeset d648ef10c5c7659ed3c9f34d5c751dc55e2c6007 by Charles Burkland in branch 'master': bpo-36144: Update os.environ and os.environb for PEP-584 (bpo-18911) https://github.com/python/cpython/commit/d648ef10c5c7659ed3c9f34d5c751dc55e2c6007

    gvanrossum commented 4 years ago

    New changeset 6d674a1bf456945eb758e85c11484a9f1494f2b4 by Brandt Bucher in branch 'master': bpo-36144: OrderedDict Union (PEP-584) (bpo-18967) https://github.com/python/cpython/commit/6d674a1bf456945eb758e85c11484a9f1494f2b4

    brandtbucher commented 4 years ago

    Three other MutableMappings we might want to update:

    Shelf is up in the air, since it doesn't look like it defines a copy() equivalent... I also have no experience with it. Since it's a MutableMapping subclass, (not a dict subclass), we could in theory hold off on updating this until someone asks for it, without backward compatibility issues.

    I think the other two should be updated, though. I can coordinate PRs for them this week.

    gvanrossum commented 4 years ago

    I definitely think we should leave Shelf alone, it's a toy class from a different era.

    It makes sense to update the weak dicts; hopefully the | and |= operators can be implemented in terms of other, more primitive operations, so we will have assurance that these classes' essential behavior is preserved.

    gvanrossum commented 4 years ago

    New changeset f393b2c588559162dc2e77f8079a42e48558870a by Curtis Bucher in branch 'master': bpo-36144: Add PEP-584 operators to collections.ChainMap (bpo-18832) https://github.com/python/cpython/commit/f393b2c588559162dc2e77f8079a42e48558870a

    gvanrossum commented 4 years ago

    New changeset 25e580a73c163f472fdeb5489bebef85da21655c by Curtis Bucher in branch 'master': bpo-36144: Add union operators to WeakKeyDictionary (bpo-19106) https://github.com/python/cpython/commit/25e580a73c163f472fdeb5489bebef85da21655c

    gvanrossum commented 4 years ago

    New changeset 8f1ed21ecf57cc8b8095d9d1058af2b9b3ed0413 by Curtis Bucher in branch 'master': bpo-36144: Add union operators to WeakValueDictionary584 (bpo-19127) https://github.com/python/cpython/commit/8f1ed21ecf57cc8b8095d9d1058af2b9b3ed0413

    brandtbucher commented 4 years ago

    And... that's it! Big thanks to everybody who had a part in making this happen.

    gvanrossum commented 4 years ago

    I'm guessing this can be closed?

    gvanrossum commented 4 years ago

    I guess we should keep this open until Raymond Hettinger has given feedback on https://github.com/python/cpython/pull/18832 (where we have the option of changing to Brandt's proposal from https://github.com/python/cpython/pull/18832#issuecomment-596910350).

    9ddc74d2-63ac-403e-9053-e681340adc0a commented 4 years ago

    First off, thanks for adding the feature, it's much appreciated.

    But it'd be great if you guys can enable list merge for the dict having list as its value, in current form I believe it's handling only "key: value" merge.

    for e.g.:
    >>> d1 = {'spam': [1, 2, 3]}
    >>> d2 = {'spam': [2, 3, 4]}
    >>> d1 | d2
    >>> {'spam': [1, 2, 3, 4]}
    brandtbucher commented 4 years ago

    Similar behavior was considered and ultimately rejected by the PEP as being too specialized:

    https://www.python.org/dev/peps/pep-0584/#concatenate-values-in-a-list

    What you're asking for it subtly different, and even *more* specialized than that. I'd recommend just subclassing dict and overriding the operator, as the PEP suggests.

    807e18dc-a104-4841-9908-4f1f279ae0a8 commented 4 years ago

    Taking the union of items() in Python 3 (viewitems() in Python 2.7) will also fail when values are unhashable objects (like lists, for example). Even if your values are hashable, since sets are semantically unordered, the behavior is undefined in regards to precedence. So don't do this:

    >>> c = dict(a.items() | b.items())
    This example demonstrates what happens when values are unhashable:
    
    >>> x = {'a': []}
    >>> y = {'b': []}
    >>> dict(x.items() | y.items())
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: unhashable type: 'list'
    Here's an example where y should have precedence, but instead the value from x is retained due to the arbitrary order of sets:
    
    >>> x = {'a': 2}
    >>> y = {'a': 1}
    >>> dict(x.items() | y.items())
    {'a': 2}

    http://net-informations.com/python/ds/merge.htm