-
-
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
base: master
Are you sure you want to change the base?
Conversation
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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.
Closes #18557