-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix #11971 #11972
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
Fix #11971 #11972
Conversation
Allows PlaceholderNode objects before raising an error for the attempted reuse of a member name inside an Enum.
basically looks good to me. cc @sobolevn |
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.
Please, add a test case (I really a fan of adding all regressions as test cases).
I found this minimal repro:
from enum import Enum
from typing import Callable, Dict
class Event(Enum):
PreAddCommand: Event = Callable[[str], None]
PostAddCommand: Event = Callable[[Dict[str, "Missing"]], None]
Note that Missing
is not defined on purpose.
Looks fine to you? 🙂
Interesting, thanks a lot @sobolevn! So is the failure I was observing related to the use of a forward reference? If so, what makes the following two cases different? PostAddCommand: Event = Callable[[Dict[str, "Entry"]], None]
PostEditCommand: Event = Callable[["Entry"], None] The |
test-data/unit/check-enum.test
Outdated
from typing import Callable, Dict | ||
|
||
class Foo(Enum): | ||
Bar: Foo = Callable[[str], None] # E: Value of type "int" is not indexable |
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.
I don't quite understand this error which mypy is raising regarding int
not being indexable. How is this relevant here?
Or is this due to some default of Enum
assuming its values are of type 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.
For completeness, this is the output of this test failing on master
@ 6c1eb5b:
E AssertionError: Unexpected type checker output (/home/max/Git/mypy/test-data/unit/check-enum.test, line 1460)
------------------------------------------------- Captured stderr call -------------------------------------------------
Expected:
main:6: error: Value of type "int" is not indexable
main:7: error: Value of type "int" is not indexable (diff)
main:7: error: Name "Missing" is not defined (diff)
Actual:
main:6: error: Value of type "int" is not indexable
main:7: error: Attempted to reuse member name "Baz" in Enum definition "Foo" (diff)
main:7: error: Value of type "int" is not indexable (diff)
main:7: error: Name "Missing" is not defined (diff)
Alignment of first line difference:
E: main:7: error: Value of type "int" is not indexable...
A: main:7: error: Attempted to reuse member name "Baz" in Enum definition "...
^
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.
Callable
is defined as Callable = 0
in our test fixtures. You can define your own type instead for tests:
class Some(Generic[T]): ...
It should not really matter.
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.
Alright then I assume it is fine as is once the CI passes. If not, please let me know
Revert this once python/mypy#11972 is merged and released.
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.
Looks like it fixes this cryptic error message.
But, I would improve a test case a bit.
Maybe it will be a good idea to move this case into pythoneval.test
? This way we will remove # E: Value of type "int" is not indexable
error message, because it will use real typing.py
Alright I moved the new test as you suggested. I tried removing the
|
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.
Awesome, thank you. I will approve it and leave it for someone else to double check and merge.
Allows PlaceholderNode objects before raising an error for the attempted reuse of a member name inside an Enum.
Description
Allows
PlaceholderNode
objects before raising an error for the attemptedreuse of a member name inside an
Enum
.Test Plan
As explained in #11971 I (unfortunately) was not yet able to find a minimal reproducing example for this.
I can verify that this fixes my scenario via which I encountered the bug, but this is obviously not sufficient.
If you have any ideas how we can reproduce this, I am happy to add a corresponding unittest.