-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Counter generic over the value #11632
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
|
||
# Test the constructor | ||
# assert_type(Counter(), Counter[Never, int]) # pyright derives "Unknown" instead of "Never" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth I think that's better than Never
. We could test the value type with something like:
for value in Counter().values:
assert_type(value, int)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this makes pyright unhappy as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_type(Counter(), "Counter[Any, int]")
works.
|
||
custom_c: Counter[str, Foo] = Counter() | ||
assert_type(custom_c, "Counter[str, Foo]") | ||
assert_type(custom_c["a"], Foo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At runtime this is actually an int though. I wonder if we need to make all these methods return _C | int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should probably not be accepted, as Counter()
is a Counter[unknown, int]
, which is incompatible with Counter[..., Foo]
. I'm not sure the test makes much sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this sort of problem apply to any Counter with a non-int value type, though? This seems like a fundamental problem with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At runtime this is actually an int though. I wonder if we need to make all these methods return _C | int.
I wonder whether type checkers support __missing__
, in which case, this should happen automatically when we add it to the stubs. But returning _C | int
makes some sense to me for getter methods.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
@erictraut: I'm unsure why pyright fails the test case as written: https://github.com/python/typeshed/actions/runs/8613668710/job/23605460935?pr=11632 custom_c = cast("Counter[str, Foo]", Counter())
# [...]
custom_c["a"] += 42 # type: ignore pyright claims that the type ignore is unnecessary here, but unless I'm missing something, both |
I'm not able to repro the problem when I copy and paste your modified definition of |
I remember that this is at least the second time, where a pyright "bug" can't be reproduced outside our CI environment. Something strange is going on with our CI it seems. |
Closes: #3438