-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Support functional API for Enum. #2805
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
Conversation
for key, value in args[1].items: | ||
if not isinstance(key, (StrExpr, UnicodeExpr)): | ||
return self.fail_enum_call_arg( | ||
"%s() with dict literal requires string literals" % class_name, call) |
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 would be better to also trigger this error in tests.
test-data/unit/pythoneval-enum.test
Outdated
[case testFunctionalEnumListOfStrings] | ||
from enum import Enum, IntEnum | ||
E = Enum('E', ('foo', 'bar')) | ||
F = Enum('F', ['bar', 'baz']) |
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 it be IntEnum
? Anyway you imported it above.
test-data/unit/pythoneval-enum.test
Outdated
[case testFunctionalEnumListOfPairs] | ||
from enum import Enum, IntEnum | ||
E = Enum('E', [('foo', 1), ['bar', 2]]) | ||
F = Enum('F', (['bar', 1], ('baz', 2))) |
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.
Probably also IntEnum
here.
mypy/semanal.py
Outdated
values.append(value) | ||
else: | ||
return self.fail_enum_call_arg( | ||
"%s() with tuple or list of (name, value) pairs not yet supported" % |
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 think this error message is misleading, since as we see from tests
E = Enum('E', [('foo', 1), ['bar', 2]])
with a list of (name, value) pairs is supported by mypy. Or am I missing something?
Thanks for the review! Addressed all points. (Ditto for the E[] PR). |
Both PRs LGTM. |
5038765
to
5a52000
Compare
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 good, but I'd like to have some additional test cases, and need to fix the merge conflict. Also, maybe the tests should be regular test cases instead of pythoneval test cases (more about this below)?
if not isinstance(callee, RefExpr): | ||
return None | ||
fullname = callee.fullname | ||
if fullname not in ('enum.Enum', 'enum.IntEnum', 'enum.Flag', 'enum.IntFlag'): |
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.
Add test cases for enum.Flag
and enum.IntFlag
?
name = cast(StrExpr, call.args[0]).value | ||
if name != var_name or self.is_func_scope(): | ||
# Give it a unique name derived from the line number. | ||
name += '@' + str(call.line) |
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.
Test anonymous Enum
that is available externally. Example:
class A:
def f(self) -> None:
E = Enum('E', 'a b')
self.x = E.a
a = A()
reveal_type(a.x)
# (Or in the nearest class if there is one.) | ||
stnode = SymbolTableNode(GDEF, info, self.cur_mod_id) | ||
if self.type: | ||
self.type.names[name] = stnode |
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.
Test Enum
defined in a class body.
if len(args) > 2: | ||
return self.fail_enum_call_arg("Too many arguments for %s()" % class_name, call) | ||
if call.arg_kinds != [ARG_POS, ARG_POS]: | ||
return self.fail_enum_call_arg("Unexpected arguments to %s()" % class_name, call) |
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.
Add test case that triggers this error.
return self.fail_enum_call_arg("Too many arguments for %s()" % class_name, call) | ||
if call.arg_kinds != [ARG_POS, ARG_POS]: | ||
return self.fail_enum_call_arg("Unexpected arguments to %s()" % class_name, call) | ||
if not isinstance(args[0], (StrExpr, UnicodeExpr)): |
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.
Add Python 2 test case that uses both str
and unicode
literals for everything.
test-data/unit/pythoneval-enum.test
Outdated
_program.py:6: error: Revealed type is 'Any' | ||
_program.py:7: error: Revealed type is 'builtins.str' | ||
|
||
[case testFunctionalEnumErrors] |
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.
Add test cases:
- Incremental checking and
Enum(...)
- Try to access missing
Enum
attribute.
What about adding a minimal stub for enum
to lib-stub
and moving these test cases away from pythoneval-enum.test
to speed them up?
@gvanrossum If you are busy, I can finish this up so that we can get this ready for the next mypy release. |
I was just about to get back to this actually, so please hold off for a few
days.
|
5a52000
to
4c731ab
Compare
Rebased (and squased all three original commits into a single new one). Next I'll try to address the review feedback. |
Oh, looks like my merge left some things broken too. |
5ea8ead
to
e7cc0cb
Compare
e7cc0cb
to
ad0ba2a
Compare
Addressed all review comments (AFAIK). |
Hm, there's a discrepancy in the type name for anonymous regular enums and anonymous functional enums in methods: one gets the class name as part of its name, the other doesn't. I suppose this is because they are processed at different times. I am going to paper this over by adjusting the test output -- let me know if you want this fixed properly. |
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 think it is better to always store the anonymous enums (and other anonymous special classes like named tuples etc.) in globals
, since storing them in names
of nearest enclosing TypeInfo
might sometimes fail, although as shown in #2931, such scenarios are super rare therefore I think it is OK to just leave it as is.
Fixes #2306.