-
-
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
Properly account for member
and nonmember
in TypeInfo.enum_members
#18559
Conversation
This comment has been minimized.
This comment has been minimized.
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 is the first time I heard about member
and nomember
, but I appreciate the opportunity to learn...
I played a little and found your fix does not cover the following slight change of your example:
from enum import Enum, member
from typing import reveal_type
class Pet(Enum):
CAT = 1
@member
def human(self) -> int:
return 2
def a(x: Pet) -> None:
if x is not Pet.human: # `human` instead of `CAT`, as in the original example
reveal_type(x) # N: Revealed type is "__main__.Pet"
Expected would be Literal[__main__.Pet.CAT]
However, this does not seem to be related to TypeInfo.enum_members
, so this comment is just a suggestion for an extension of this or an independent PR.
Because of my lack of experience, I am not really sure if some edge cases are missed, but to me, this change looks good and is definitely an improvement.
Yeah, there are multiple other problems with I think that the example that you've mentioned is happening because right now |
( | ||
isinstance(sym.node, Var) | ||
and name not in EXCLUDED_ENUM_ATTRIBUTES | ||
and not name.startswith("__") |
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.
Should this condition exist for enum.member too because #18563?
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.
Hm, good question. Let's keep it for now, I will change it in #18563 if needed.
Diff from mypy_primer, showing the effect of this PR on open source code: freqtrade (https://github.com/freqtrade/freqtrade): 1.98x slower (128.9s -> 255.0s in a single noisy sample)
|
My further plan: get rid of |
Done: #18675 |
…rs` (python#18559) Closes python#18557 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Closes #18557