bpo-36144: Dictionary Union (PEP 584) #12088

brandtbucher · 2019-02-28T04:19:13Z

https://bugs.python.org/issue36144

`d1 + d2` is semantically identical to `d3 = d1.copy(); d3.update(d2); d3`, and `d1 += d2` is semantically identical to `d1.update(d2)`. Isn't that sweet?

It feels so weird to sum dicts like this... cool.

This gets the collections tests passing again.

This adds one test with 5 new assertions.

In case this gets pulled. Very cool!

Whoops.

This whole patch adds 3 new objects with docstrings: dict.__add__, dict.__radd__, dict.__iadd__. On some builds, this puts us over the threshold, so bump the count!

rhettinger · 2019-02-28T06:35:02Z

It usually isn't an effective use to time to go straight to a PR before a proposal has been discussed and accepted.

brandtbucher · 2019-02-28T15:46:02Z

I figured it would help with decision-making. The person who started the thread asked for an implementation, and I had the free time to make one. I'm not very familiar with how the PEP process usually works, but I saw in PEP 1:

Standards Track PEPs consist of two parts, a design document and a reference implementation. It is generally recommended that at least a prototype implementation be co-developed with the PEP, as ideas that sound good in principle sometimes turn out to be impractical when subjected to the test of implementation.

Sorry if this is a distraction. It's not my intention to disrupt the normal process!

Replaces two places where a failed PyDict_Update call returns NotImplemented instead of raising the error.

rhettinger · 2019-02-28T23:32:00Z

It looks like this may yet have a chance :-)

This brings this reference implementation in line with PEP 584's spec. Separately, allows RHS of += to be any valid arg to dict.update.

These are implemented as they are currently laid out in PEP 584.

This should get tests passing again.

And... we're officially implemented!

…nto addiction

This fixes issues where iteration errors could be swallowed, and also stops a possible reference leak.

This also implements the complex keys/__iter__ dance that update uses, as an example of compatibility with dict.update.

dict.__iadd__ can take any mapping (i.e., a list).

This is pretty much copied-and-pasted from UserDict, but it's better than what we had before. They may need some tweaking.

This was requested by Steven for comparison purposes. The final implementation will only keep one!

brandtbucher · 2020-02-06T07:09:34Z

I've added subclass tests.

serhiy-storchaka · 2020-02-06T08:07:32Z

Because list and set do not call copy() and extend()/update().

It would make dict very different from other builtin types.

gvanrossum · 2020-02-06T15:35:02Z

I agree with Serhiy.

On Thu, Feb 6, 2020 at 00:07 Serhiy Storchaka ***@***.***> wrote: Because list and set do not call copy() and extend()/update(). It would make dict very different from other builtin types. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12088?email_source=notifications&email_token=AAWCWMUS4RJZZ5BMOQH224DRBPAMNA5CNFSM4G2X5HR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK6JJ6Q#issuecomment-582784250>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWCWMULOM22F7PTNNNJGM3RBPAMNANCNFSM4G2X5HRQ> .

-- --Guido (mobile)

serhiy-storchaka · 2020-02-06T15:55:16Z

Since it is a principal point of the PEP, I suggest you to create two or more separate PRs: one does not depend on subclasses, and the other calls copy() and update().

Yet one argument against calling copy() and update() is that it would be the only case when operator depends on non-dunder methods.

brandtbucher · 2020-02-06T16:34:57Z

@serhiy-storchaka Yes, and in the earlier stages of the proposal I felt as you did:

https://mail.python.org/archives/list/python-ideas@python.org/message/HBWBRRGXUGPT2NQLVNM3XLBHZSHKWBGI/

But Steven was strongly for this, and over time his arguments were enough to shift my view.

https://mail.python.org/archives/list/python-ideas@python.org/message/SIPJZPEUTIMZ7ETCCDUKWDIFNRN7YHMH/

Also:

Possibly relevant: I've always been frustrated and annoyed at classes
that hardcode their own type into methods... This gets really annoying really quickly. Try subclassing int, for example, where you have to override something like 30+ methods and do nothing but wrap calls to super.

It's more common to subclass dict than either list or set, so I see this as more important issue than just blindly following what seems like a less-than-ideal precedent without a really good reason to.

dict doesn't have any operators yet, so we're not bound by any prior design constraints in this area (rather, we're setting them). My example above with defaultdict shows that this has immediate benefits for both usability and maintenance.

brandtbucher · 2020-02-06T16:35:03Z

@gvanrossum, is this enough of an issue that you would consider rejecting the proposal over it? I know that Steven and I have somewhat strongly formed opinions on this from all of the endless mailing-list threads, so it might be a good idea to continue this discussion on the python-dev thread where he can participate, if so.

brandtbucher · 2020-02-06T16:40:45Z

Also, @serhiy-storchaka it would be pretty easy to just strip the last 5 commits at this point. I can make another PR if the two designs deviate more substantially.

gvanrossum · 2020-02-06T16:55:16Z

@brandtbucher This won't be an issue unless you make it one. The proposal is a good one regardless of what it does with subclasses. Does the PEP explicitly address this (either in the spec or in the text about rejected alternatives or elsewhere)? If so can you point me to the text?

A general problem with honoring subclasses is that it's worse when it's done inconsistently than not at all. For immutable objects (e.g. Steven's int subclassing example) the problem is that you won't know the arguments to the subclass constructor, and you can't mutate int objects after they've been created. So you just have to suck it up. For dict, in this case copy()+update() will work. But that's just luck. C code that inserts a new key into a dict shouldn't have to look for an overridden __setitem__, it should be able to just use PyDict_SetItem.

brandtbucher · 2020-02-06T17:23:06Z

I don't believe this was ever explicitly outlined in the PEP (probably because we didn't get much pushback relative to other decisions 😉), but the Reference Implementation section is clear - it uses bound self methods rather than unbound dict methods. We also mention the desire of subclass-preserving behavior in the Motivation section, although that is less relevant because the operators could still be manually overridden.

Thinking out loud, if dict were to grow more operators, they would likely be &, ^, and - (with the associated in-place versions, which is important). Continuing our subclass-friendliness started here would probably require, at the very least, also respecting __delitem__ and __setitem__ as well...

How do we feel about keeping the copy call and dropping the update one? I know Steven was open to that. New subclasses just have to implement copy to get fast, working versions of everything, and we get to keep a working defaultdict, etc...

gvanrossum · 2020-02-06T18:25:03Z

I still think it's a doubtful choice (and the PEP is clear that the code there is not exactly equivalent). Can you bring this up in the python-dev thread? So far you've convinced 0 out of 2 core devs -- but you still could get the Steering Council behind you.

gvanrossum · 2020-02-06T19:08:12Z

Sorry, one out of three. :-)

brandtbucher · 2020-02-06T19:49:23Z

Can you bring this up in the python-dev thread?

Done.

gvanrossum · 2020-02-06T23:05:44Z

This happened right?

brandtbucher · 2020-02-06T23:07:39Z

Yes, I commented in the new PEP thread:

https://mail.python.org/archives/list/python-dev@python.org/message/Z5SSGCH736UWHKFGJEZROH6VKPKXIT7W/

brandtbucher · 2020-02-07T00:00:11Z

I am going to go ahead and remove the update calls, since we've come to an agreement on that. Jury's still out on copy.

gvanrossum

LGTM. Obviously let's wait until PEP 584 is accepted.

brandtbucher · 2020-02-17T16:16:59Z

Thanks for all of the feedback @gvanrossum and @serhiy-storchaka (especially for your insightful comments on the subclass issue).

brandtbucher · 2020-02-25T00:43:41Z

Guido just informed us that the Steering Council accepted his recommendation!

bedevere-bot · 2020-02-25T03:47:37Z

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

brandtbucher added 6 commits February 27, 2019 18:58

Create dict_add and dict__iadd dict methods.

9397506

`d1 + d2` is semantically identical to `d3 = d1.copy(); d3.update(d2); d3`, and `d1 += d2` is semantically identical to `d1.update(d2)`. Isn't that sweet?

Fix failing "sum" test.

c69108a

It feels so weird to sum dicts like this... cool.

Update collections.UserDict.

dd42f4f

This gets the collections tests passing again.

Update test_dict.py.

f59cdab

This adds one test with 5 new assertions.

Update ACKS.

d895b9b

In case this gets pulled. Very cool!

Fixed whitespace.

da8e2e3

Whoops.

brandtbucher requested a review from rhettinger as a code owner February 28, 2019 04:19

the-knights-who-say-ni added the CLA signed label Feb 28, 2019

bedevere-bot added the awaiting review label Feb 28, 2019

brandtbucher and others added 2 commits February 27, 2019 20:51

Update doctest count for builtins.

91d6466

This whole patch adds 3 new objects with docstrings: dict.__add__, dict.__radd__, dict.__iadd__. On some builds, this puts us over the threshold, so bump the count!

📜🤖 Added by blurb_it.

b2eced1

rhettinger added the DO-NOT-MERGE label Feb 28, 2019

Fix error handling.

bf0b383

Replaces two places where a failed PyDict_Update call returns NotImplemented instead of raising the error.

rhettinger removed the DO-NOT-MERGE label Feb 28, 2019

brandtbucher and others added 6 commits March 2, 2019 09:53

Be "nicer" to subclasses than other built-in types.

e660046

This brings this reference implementation in line with PEP 584's spec. Separately, allows RHS of += to be any valid arg to dict.update.

Add dict subtraction operations.

4c0223b

These are implemented as they are currently laid out in PEP 584.

Add new subtraction methods to collections.UserDict.

2cb1629

This should get tests passing again.

Add new dict subtraction tests.

05ed4cf

And... we're officially implemented!

📜🤖 Added by blurb_it.

594ae27

Merge branch 'addiction' of https://github.com/brandtbucher/cpython i…

d1c830c

…nto addiction

brandtbucher changed the title ~~bpo-36144: Dictionary addition.~~ bpo-36144: Dictionary Addition/Subtraction (PEP 584) Mar 2, 2019

methane added the DO-NOT-MERGE label Mar 3, 2019

brandtbucher added 5 commits March 3, 2019 14:59

Better error handling and "keys" method lookup.

f7ed0a2

This fixes issues where iteration errors could be swallowed, and also stops a possible reference leak.

Clean up comments, names, and logic.

8be454a

This also implements the complex keys/__iter__ dance that update uses, as an example of compatibility with dict.update.

Update commented code.

63c36dd

dict.__iadd__ can take any mapping (i.e., a list).

Update new collections.UserDict operators.

2ba0580

This is pretty much copied-and-pasted from UserDict, but it's better than what we had before. They may need some tweaking.

Add | and |= operators as aliases to + and +=.

fa44826

This was requested by Steven for comparison purposes. The final implementation will only keep one!

Strip whitespace.

f09ba7a

brandtbucher added 2 commits February 6, 2020 16:09

Remove calls to "update" method when merging.

ec33664

Revert subclass-preserving stuff.

4afd3b6

gvanrossum approved these changes Feb 17, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Feb 17, 2020

brandtbucher removed the DO-NOT-MERGE label Feb 25, 2020

gvanrossum merged commit eb8ac57 into python:master Feb 25, 2020

bedevere-bot removed the awaiting merge label Feb 25, 2020

Queenmariehatcher approved these changes May 28, 2021

View reviewed changes

brandtbucher deleted the addiction branch July 21, 2022 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36144: Dictionary Union (PEP 584) #12088

bpo-36144: Dictionary Union (PEP 584) #12088

brandtbucher commented Feb 28, 2019 •

edited by bedevere-bot

Loading

rhettinger commented Feb 28, 2019

brandtbucher commented Feb 28, 2019

rhettinger commented Feb 28, 2019

brandtbucher commented Feb 6, 2020

serhiy-storchaka commented Feb 6, 2020

gvanrossum commented Feb 6, 2020 via email

serhiy-storchaka commented Feb 6, 2020

brandtbucher commented Feb 6, 2020 •

edited

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

brandtbucher commented Feb 6, 2020

gvanrossum commented Feb 6, 2020

brandtbucher commented Feb 6, 2020 •

edited

Loading

gvanrossum commented Feb 6, 2020

gvanrossum commented Feb 6, 2020 via email •

edited

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

gvanrossum commented Feb 6, 2020 via email •

edited

Loading

brandtbucher commented Feb 6, 2020

brandtbucher commented Feb 7, 2020

gvanrossum left a comment

brandtbucher commented Feb 17, 2020

brandtbucher commented Feb 25, 2020

bedevere-bot commented Feb 25, 2020

bpo-36144: Dictionary Union (PEP 584) #12088

bpo-36144: Dictionary Union (PEP 584) #12088

Conversation

brandtbucher commented Feb 28, 2019 • edited by bedevere-bot Loading

rhettinger commented Feb 28, 2019

brandtbucher commented Feb 28, 2019

rhettinger commented Feb 28, 2019

brandtbucher commented Feb 6, 2020

serhiy-storchaka commented Feb 6, 2020

gvanrossum commented Feb 6, 2020 via email

serhiy-storchaka commented Feb 6, 2020

brandtbucher commented Feb 6, 2020 • edited Loading

brandtbucher commented Feb 6, 2020 • edited Loading

brandtbucher commented Feb 6, 2020

gvanrossum commented Feb 6, 2020

brandtbucher commented Feb 6, 2020 • edited Loading

gvanrossum commented Feb 6, 2020

gvanrossum commented Feb 6, 2020 via email • edited Loading

brandtbucher commented Feb 6, 2020 • edited Loading

gvanrossum commented Feb 6, 2020 via email • edited Loading

brandtbucher commented Feb 6, 2020

brandtbucher commented Feb 7, 2020

gvanrossum left a comment

Choose a reason for hiding this comment

brandtbucher commented Feb 17, 2020

brandtbucher commented Feb 25, 2020

bedevere-bot commented Feb 25, 2020

brandtbucher commented Feb 28, 2019 •

edited by bedevere-bot

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

gvanrossum commented Feb 6, 2020 via email •

edited

Loading

brandtbucher commented Feb 6, 2020 •

edited

Loading

gvanrossum commented Feb 6, 2020 via email •

edited

Loading