Add typing.Counter and typing.ChainMap #366
Conversation
python2/typing.py
Outdated
| return _generic_new(collections.deque, cls, *args, **kwds) | ||
|
|
||
|
|
||
| class Counter(collections.Counter, MutableSequence[T]): |
There was a problem hiding this comment.
I believe this should be Dict[T, int] instead
There was a problem hiding this comment.
Yes, sorry for missing that.
src/typing.py
Outdated
| return _generic_new(collections.deque, cls, *args, **kwds) | ||
|
|
||
|
|
||
| class Counter(collections.Counter, MutableSequence[T], extra=collections.Counter): |
There was a problem hiding this comment.
Dict[T, int], see above comment.
|
I am not against this. If others are also not against this, then I think we could add these two new generic collections. |
| # ChainMap only exists in 3.3+ | ||
| __all__.append('ChainMap') | ||
|
|
||
| class ChainMap(collections.ChainMap, MutableMapping[KT, VT], |
There was a problem hiding this comment.
typeshed defines this as Dict[KT, VT], I am not sure this is right, but typing and typeshed should be in sync for sure.
There was a problem hiding this comment.
I think MutableMapping is right; ChainMap is not a subclass of dict. I'll submit that change to typeshed.
python2/typing.py
Outdated
| def __new__(cls, *args, **kwds): | ||
| if _geqv(cls, Counter): | ||
| raise TypeError("Type Counter cannot be instantiated; " | ||
| "use Counter() instead") |
There was a problem hiding this comment.
I would use full name collections.Counter() in the second line, otherwise this error message reads unclear.
(The same applies to Python 3 version).
|
|
||
| def test_counter(self): | ||
| self.assertIsSubclass(collections.Counter, typing.Counter) | ||
|
|
There was a problem hiding this comment.
Please add a test checking that a subclass of counter could be instantiated, as you do for ChainMap in test_chainmap_subclass. (And also for Python 3)
|
Thank you! |
|
@gvanrossum Could you please take a look at this PR and python/typeshed#878? I think the PRs are ready, but I would like to have your approval. |
|
@JelleZijlstra could you please update the PR allowing instantiation of |
|
Nice, thanks! Now we'll no longer need both |
Pretty much copied from deque.
Only to the Python 3 version because ChainMap was added in 3.3.
Apparently typing still supports 3.2
JelleZijlstra
force-pushed
the
counter
branch
from
0d47706 to
3dfbaf9
Compare
|
Rebased the branch and changed it to allow instantiation. |
src/test_typing.py
Outdated
| def test_chainmap_type_erasure_special(self): | ||
| class MyChain(typing.ChainMap[str, T]): ... | ||
| self.assertIs(MyChain[int]().__class__, MyChain) | ||
| self.assertIs(MyChain[int]().__orig_class__, MyChain[int]) |
There was a problem hiding this comment.
One last thing, you use assertIs here that relies on generic type cache. This is a bit dangerous if the test is run in refleak hunting mode (-R). I see two options here:
- change this to use
assertEqual - add
self.clear_caches()at beginning and edit the comment in the previoustest_type_erasure_specialto say something like# this test and the next one are only two that rely on type caching
Sorry for not noticing this before.
There was a problem hiding this comment.
Note that assertIs in other places in your PR is OK since it does not depend on generic type caching.
There was a problem hiding this comment.
I copied this from similar tests for defaultdict above; should I change those too?
There was a problem hiding this comment.
No, that one is intentional. (note that it is already "protected" by self.clear_caches())
|
I am happy to see this go in. (I haven't done a detailed code review, but I trust the two of you.) Once this goes in we need additional changes in typeshed, right? |
Yes, @JelleZijlstra would you prefer to make a PR for typeshed? |
|
Yes, I can do that later today or tomorrow. Thanks for merging it in! |
This implements python/typing#366 in mypy.
4 participants
Similar to deque, which was added recently.