bpo-36144: Add union operators to WeakKeyDictionary #19106
Conversation
Added `_ior_`, `__or__`, and `__ror__` methods to WeakKeyDictionary and added corresponding tests to test_weakref.py.
Removed unecessary isinstance from __ior__
Added tests and modified .__ror__ behavior
|
Thanks for your help with these @curtisbucher! The macOS test failure looks unrelated (a bunch of other PR's have the same test failing). Let's get this one accepted before opening another PR for |
gvanrossum
changed the title
Lib/weakref.py
Outdated
| return self | ||
|
|
||
| def __or__(self, other): | ||
| if isinstance(other, (dict, WeakKeyDictionary, WeakValueDictionary)): |
There was a problem hiding this comment.
What's the value of making WeakKeyDict | WeakValueDict return a WeakKeyDict? If that, why not any other Mapping? I can't think of a use case for mixing these like this.
There was a problem hiding this comment.
So would it make more sense to have | accept any mapping and return a WeakKeyDict?
Lib/weakref.py
Outdated
| return NotImplemented | ||
|
|
||
| def __ror__(self, other): | ||
| if isinstance(other, (dict, WeakKeyDictionary, WeakValueDictionary)): |
There was a problem hiding this comment.
This seems even more doubtful -- why would WeakValueDict | WeakKeyDict return a WeakKeyDict instead of a WeakValueDict? Aren't they symmetrical? (Also there are no tests for these mixes.)
There was a problem hiding this comment.
When __ror__ is used, should it always return the same type as other?
There was a problem hiding this comment.
No, it should return the type of self. If other had wanted to be the return type, that class could have done so in its __or__ method, which is called before __ror__. But apparently they didn't, so it's ours.
(Here's a clarifying example for why it should be this way: Consider 1 + 2.5. int.__or__ returns NotImplemented when other is a float, so then float.__ror__ is invoked with other set to 1, and it returns the float value 3.5!)
|
How about just |
Hm, yeah, that sounds reasonable, and it seems to be what we're doing for ChainMap and UserDict. (It seems there's a dichotomy: for classes that derive from dict we are strict, and for other classes we allow any Mapping. This seems a reasonable compromise -- maybe we can add this as a note to the PEP?) |
Allow WeakKeyDictionary union with any mapping.
|
@gvanrossum: Please replace |
|
Congrats on another great patch @curtisbucher ! |
The
__ror__implementation was a little tricky, because we can't assume that a subclass will have the same constructor signature as the parent class, since we don't do that anywhere else.@brandtbucher
https://bugs.python.org/issue36144