-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Always use .enum_members
to find enum members
#18675
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
Diff from mypy_primer, showing the effect of this PR on open source code: freqtrade (https://github.com/freqtrade/freqtrade): 2.00x slower (129.2s -> 257.9s in a single noisy sample)
|
I can confirm the test change is fully valid, bare I'm glad to see it fixes one of the problems known to me. Since it's likely the same place, can we by any chance make mypy recognize |
Ough. Something is wrong with test stubs. Given the following code: # flags: --python-version 3.12 --warn-unreachable
from enum import Enum, member, nonmember
from typing import Literal, Never
def assert_never(_: Never) -> Never: ...
class E2(Enum):
@member
def C() -> None: ... # E: Method must have at least one argument. Did you forget the "self" argument? [misc]
c: Literal[E2.C] # E: Parameter 1 of Literal[...] is invalid [valid-type]
def check_2(e: E2) -> None:
match e:
case E2.C:
pass
case other:
assert_never(other) # E: Argument 1 to "assert_never" has incompatible type "E2"; expected "Never" [arg-type]
if e is E2.C:
pass
else:
assert_never(e) # E: Argument 1 to "assert_never" has incompatible type "<subclass of "enum.member[Callable[[], None]]" and "__main__.E2">"; expected "Never" [arg-type] If I
but putting the same in a test with
It's either the stubs or something shady with import resolution, but anyway enum tests we're running now may not represent actual |
I will fix the stubs in the following PR, thanks for finding this! 👍 |
Does the stub issue impact this PR, i.e. should we wait for the stubs to be fixed before reviewing this PR? |
@sterliakov sorry, I don't quite understand your example: where can I find a test case for |
I'm not sure there is already a corresponding test, I just checked out your branch to try it against enum problems I encountered recently, added a test from that code and observed critical discrepancies between bare Failing testcase (added to
The same code pasted into a plain file produces mypy output matching the comments (I have current branch installed editable,
I'm not sure how critical this problem is - in my understanding this means that some enum tests may actually deviate from And the expected |
("method must have at least one argument" is also troubling, because |
This very issue likely doesn't affect any existing tests as |
@sterliakov can we create a new issue, as this seems like a different problem? |
Closes #18565
This fixes the problem with
nonmember
andmember
special cases, however, it required me to change one test case.See
testEnumReachabilityPEP484ExampleSingletonWithMethod
change, because in runtimetoken
is not a member by default, at least in recent python versions. Proof:and
So, I had to add
member()
there to make the test pass.I will continue to improve enums support in the future :)
For example, we need to support
_ignore_
values and some other corner cases.