Hi Steven. Thanks for the clarifications. I've pushed a complete working patch (with tests) to GitHub. It's linked to the bpo issue. Branch: https://github.com/brandtbucher/cpython/tree/addiction PR: https://github.com/python/cpython/pull/12088 Right now, it's pretty much a straight reimplementation of your Python examples. I plan to update it periodically to keep it in sync with any changes, and to make a few optimizations (for example, when operands are identical or empty). Let me know if you have any questions/suggestions. Stoked to learn and help out with this process! :) Brandt On Fri, Mar 1, 2019 at 7:57 PM Steven D'Aprano <steve@pearwood.info> wrote:
Executive summary:
- I'm going to argue for subclass-preserving behaviour;
- I'm not wedded to the idea that dict += should actually call the update method, so long as it has the same behaviour;
- __iadd__ has no need to return NotImplemented or type-check its argument.
Details below.
On Fri, Mar 01, 2019 at 04:10:44PM -0800, Brandt Bucher wrote:
[...]
In your Python implementation samples from the PEP, dict subclasses will behave differently from how list subclasses do. List subclasses, without overrides, return *list* objects for bare "+" operations
Right -- and I think they are wrong to do so, for reasons I explained here:
https://mail.python.org/pipermail/python-ideas/2019-March/055547.html
I think the standard handling of subclasses in Python builtins is wrong, and I don't wish to emulate that wrong behaviour without a really good reason. Or at least a better reason than "other methods break subclassing unless explicitly overloaded, so this should do so too".
Or at least not without a fight :-)
(and "+=" won't call an overridden "extend" method).
I'm slightly less opinionated about that. Looking more closely into the docs, I see that they don't actually say that += calls list.extend:
s.extend(t) extends s with the contents of t (for or s += t the most part the same as s[len(s):len(s)] = t)
https://docs.python.org/3/library/stdtypes.html#mutable-sequence-types
only that they have the same effect. So the wording re lists calling extend certainly needs to be changed. But that doesn't mean that we must change the implementation. We have a choice:
- regardless of what lists do, we define += for dicts as literally calling dict.update; the more I think about it, the less I like this.
- Or we say that += behaves similarly to update, without actually calling the method. I think I prefer this.
(The second implies either that += either contains a duplicate of the update logic, or that += and update both delegate to a private, C-level function that does most of the work.)
I think that the second approach (define += as having the equivalent semantics of update but without actually calling the update method) is probably better. That decouples the two methods, allows subclasses to change one without necessarily changing the other.
So a more analogous pseudo-implementation (if that's what we seek) would look like:
def __add__(self, other): if isinstance(other, dict): new = dict.copy(self) dict.update(new, other) return new return NotImplemented
We should not require the copy method.
The PEP should be more explicit that the approximate implementation does not imply the copy() and update() methods are actually called.
def __iadd__(self, other): if isinstance(other, dict): dict.update(self, other) return self return NotImplemented
I don't agree with that implementation.
According to PEP 203, which introduced augmented assignment, the sequence of calls in ``d += e`` is:
1. Try to call ``d.__iadd__(e)``.
2. If __iadd__ is not present, try ``d.__add__(e)``.
3. If __add__ is missing too, try ``e.__radd__(d)``.
but my tests suggest this is inaccurate. I think the correct behaviour is this:
1. Try to call ``d.__iadd__(e)``.
2. If __iadd__ is not present, or if it returns NotImplemented, try ``d.__add__(e)``.
3. If __add__ is missing too, or if it returns NotImplemented, fail with TypeError.
In other words, e.__radd__ is not used.
We don't want dict.__iadd__ to try calling __add__, since the later is more restrictive and less efficient than the in-place merge. So there is no need for __iadd__ to return NotImplemented. It should either succeed on its own, or fail hard:
def __iadd__(self, other): self.update(other) return self
Except that the actual C implementation won't call the update method itself, but will follow the same semantics.
See the docstring for dict.update for details of what is accepted by update.
-- Steven _______________________________________________ Python-ideas mailing list Python-ideas@python.org https://mail.python.org/mailman/listinfo/python-ideas Code of Conduct: http://python.org/psf/codeofconduct/