-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Makes Enum members implicitly final, refs #5599 #10852
Conversation
This comment has been minimized.
This comment has been minimized.
This one is interesting: class A(str, Enum):
value = 'value' Produces:
|
This comment has been minimized.
This comment has been minimized.
Is this fail related?
It does not seem to be. |
It's unrelated, @pranavrajpal fixed this in #10853 |
Rebased, now it should be fine. |
This comment has been minimized.
This comment has been minimized.
I was just intending to implement this myself, but luckily I checked before and saw there's already a PR for it :) My reason for wanting this is not so much marking the values immutable, but to have class A(Enum):
some: Final = 'some'
other = 'other'
reveal_type(A.some.value) # Literal['some']
reveal_type(A.other.value) # str but no reason for it to not be implicit. |
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.
Nice, enum members are clearly not mutable. This looks good to me overall, but I have question about one change.
proper_type is not None and is_equivalent(proper_type, underlying_type) | ||
for proper_type in proper_types) | ||
if all_equivalent_types: | ||
return make_simplified_union(cast(Sequence[Type], proper_types)) |
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.
It's unclear to me when the above new code is needed. Is there a test case that depends on this? If not, could you add a test case.
In any case, it would be good to have a comment here.
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.
Here are related changes:
- https://github.com/python/mypy/pull/10852/files#diff-ebc735ad8fa6eb42d5390fa4fa9ef60581764c7bbfb5eafcde16f5e40c330c84R69
- https://github.com/python/mypy/pull/10852/files#diff-ebc735ad8fa6eb42d5390fa4fa9ef60581764c7bbfb5eafcde16f5e40c330c84R143
- https://github.com/python/mypy/pull/10852/files#diff-ebc735ad8fa6eb42d5390fa4fa9ef60581764c7bbfb5eafcde16f5e40c330c84R102
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.
Updated code with more comments 👍
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.
Thanks, this is a nice improvement. Lgtm, but wouldn't mind if Jukka took another look.
This comment has been minimized.
This comment has been minimized.
Rebased! 🚜 |
This comment has been minimized.
This comment has been minimized.
Merge conflicts 😮💨 Should be fine now! |
Diff from mypy_primer, showing the effect of this PR on open source code: prefect (https://github.com/PrefectHQ/prefect.git)
+ src/prefect/core/task.py:45: error: Cannot override writable attribute "value" with a final one
alerta (https://github.com/alerta/alerta.git)
+ alerta/models/enums.py:159: error: Cannot override writable attribute "value" with a final one
|
lgtm, I really like this feature! Also thanks for your detailed comments. |
Awesome, thanks, everyone! 🎉 |
It looks like this still doesn't work in latest mypy for strings (but does work for int/bool). Any idea why? |
refs python#5599 This change allows to catch this error by making all Enum members implicitly Final. Also modifies Enum plugin, since it was not ready to work with `Literal[True]` and `Literal[False]`. Co-authored-by: Shantanu <[email protected]>
Makes
Enum
members implicitlyfinal
My problem was that this code was typechecking:
While in runtime it was failing with:
This change allows to catch this error by making all
Enum
members implicitlyFinal
.I also had to modify
Enum
plugin, since it was not ready to work withLiteral[True]
andLiteral[False]
.Closes #5599
Notes
I would love to hear your feedback! 👍