Collections that inherit from dict should should override copy() #1856
Conversation
As should __copy__
| @@ -93,10 +93,13 @@ class Counter(Dict[_T, int], Generic[_T]): | |||
| def __iand__(self, other: Counter[_T]) -> Counter[_T]: ... | |||
| def __ior__(self, other: Counter[_T]) -> Counter[_T]: ... | |||
|
|
|||
| _OrderedDictT = TypeVar('_OrderedDictT', bound=OrderedDict) | |||
There was a problem hiding this comment.
Personally, I prefer shorter names for type variables (also this is what PEP 8 currently recommends).
There was a problem hiding this comment.
I followed the convention already established in the file by _UserDictT, _UserListT, and _UserStringT. I think it makes sense to keep it consistent.
There was a problem hiding this comment.
If you really hate it as is I can do _OdictT. your call.
stdlib/2/collections.pyi
Outdated
| class OrderedDict(Dict[_KT, _VT], Reversible[_KT], Generic[_KT, _VT]): | ||
| def popitem(self, last: bool = ...) -> Tuple[_KT, _VT]: ... | ||
| def __reversed__(self) -> Iterator[_KT]: ... | ||
| def __copy__(self) -> OrderedDict[_KT, _VT]: ... | ||
| def __copy__(self: _OrderedDictT) -> _OrderedDictT: ... |
There was a problem hiding this comment.
There is actually no Copyable protocol in typing, so I would say it is safe to remove it, if it is not defined at runtime.
stdlib/2/collections.pyi
Outdated
| class OrderedDict(Dict[_KT, _VT], Reversible[_KT], Generic[_KT, _VT]): | ||
| def popitem(self, last: bool = ...) -> Tuple[_KT, _VT]: ... | ||
| def __reversed__(self) -> Iterator[_KT]: ... | ||
| def __copy__(self) -> OrderedDict[_KT, _VT]: ... | ||
| def __copy__(self: _OrderedDictT) -> _OrderedDictT: ... | ||
| def copy(self: _OrderedDictT) -> _OrderedDictT: ... |
There was a problem hiding this comment.
Note that there are some bugs with self-types in generic types (in mypy). But hopefully most of them will be fixed soon (as soon as I will have time to finish a patch I am working on).
|
Ok, I removed I also added |
|
I guess I should add |
|
You can add other copy methods in this PR using the current local |
Will do.
I'm probably the wrong guy to head up that PR because I personally find short TypeVar names to be too cryptic, and I independently adopted the same convention used in collections.pyi (especially for self-types). I've always been taught to favor clarity over brevity. |
|
For what it's worth, I think slightly longer TypeVar names are fine here. |
Counter also inherits from dict and so needs its own copy with self-type.
|
This PR now includes |
Solves issue #1642. A previous attempt at this, #1656, added
__copy__but omittedcopy, and it did not properly use self-type.One question regarding the previous PR: should the
__copy__method be in the stubs at all? As far as I can tell,OrderedDictdoes not have a__copy__method. Was adding this a mistake or is it necessary for structural type checking of some sort?