Skip to content
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

Use EnumMeta instead of Enum to mark enum classes #4319

Merged
merged 10 commits into from
Apr 10, 2018

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Dec 4, 2017

Implement #4311.

The tests will fail until python/typeshed#1770 is synched.

@emmatyping emmatyping mentioned this pull request Dec 4, 2017
Copy link
Collaborator

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of requests.

main:7: error: Incompatible types in assignment (expression has type "int", variable has type "Medal")
m = 1 # E: Incompatible types in assignment (expression has type "int", variable has type "Medal")

[case testEnumFromEnumMetaBasics]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to have a testcase for something that inherits from a class that has EnumMeta as a metaclass, to check those classes are still found to be Enums for all intents and purposes.

Also perhaps a test of a EnumMeta deriving class that tries to use generics?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to have a testcase for something that inherits from a class that has EnumMeta as a metaclass, to check those classes are still found to be Enums for all intents and purposes.

Will do, but note that this is already handled by classes that inherit Enum itself.

I'm not sure I understand the second suggestion. Should mypy support something like that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on the first, but just to be safe. Also on the second, I mean to check that it fails correctly :)

@emmatyping
Copy link
Collaborator

Hm, I think you need to git submodule update to pull the latest changes from typeshed.

@elazarg
Copy link
Contributor Author

elazarg commented Feb 25, 2018

git submodule update doesn't do the trick for some reason. I tried git checkout upstream/master typeshed which feels horribly wrong, but seems to work.

bronze = None
reveal_type(Medal.bronze) # E: Revealed type is '__main__.Medal'
m = Medal.gold
m = 1 # E: Incompatible types inassignment (expression has type "int", variable has type "Medal")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a space between "in" and "assignment"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Sorry about that.

Copy link
Collaborator

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fast fixes.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool how much code can be deleted!

mypy/semanal.py Outdated
@@ -1275,6 +1273,11 @@ def analyze_metaclass(self, defn: ClassDef) -> None:
# do not declare explicit metaclass, but it's harder to catch at this stage
if defn.metaclass is not None:
self.fail("Inconsistent metaclass structure for '%s'" % defn.name, defn)
else:
if defn.info.metaclass_type.type.fullname() == 'enum.EnumMeta':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the metaclass is a subclass of EnumMeta, it's not an enum? (I'm not sure I care, but I wonder if this is an intentional choice.)


[case testEnumFromEnumMetaBasics]
from enum import EnumMeta
class Medal(metaclass=EnumMeta):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code doesn't actually work -- you need to also inherit from enum.Enum, else the class definition fails at runtime with

TypeError: Medal().__init__() takes no arguments

(Note: I'm not asking that mypy enforce this -- but I still think the test should work as an example.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to do about it. If Medal inherits from Enum, the test doesn't check for what I want it to check. We might as well drop it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a def __init__(self, *args): pass constructor, if you deem it better. It works, though might be considered an implementation detail.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say add a comment explaining that it doesn't work at runtime and what you are testing for.


[case testEnumFromEnumMetaSubclass]
from enum import EnumMeta
class Achievement(metaclass=EnumMeta): pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must also inherit from enum.Enum.

@msullivan msullivan merged commit 6f11f35 into python:master Apr 10, 2018
@JelleZijlstra
Copy link
Member

This seems to have broken typeshed CI. https://travis-ci.org/python/typeshed/jobs/364931878 I'll look into it now.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Apr 11, 2018
This fixes an error in Travis that seems to have been caused by python/mypy#4319.

The fix was taken from the stdlib/3.4/enum.pyi stub. Mypy no longer assumes
that classes whose metaclass is EnumMeta are subclasses of Enum, so we can't
bound the typevar on Enum.
JelleZijlstra added a commit to python/typeshed that referenced this pull request Apr 11, 2018
This fixes an error in Travis that seems to have been caused by python/mypy#4319.

The fix was taken from the stdlib/3.4/enum.pyi stub. Mypy no longer assumes
that classes whose metaclass is EnumMeta are subclasses of Enum, so we can't
bound the typevar on Enum.
gvanrossum pushed a commit to python/typeshed that referenced this pull request Apr 11, 2018
This fixes an error in Travis that seems to have been caused by python/mypy#4319.

The fix was taken from the stdlib/3.4/enum.pyi stub. Mypy no longer assumes
that classes whose metaclass is EnumMeta are subclasses of Enum, so we can't
bound the typevar on Enum.
yedpodtrzitko pushed a commit to yedpodtrzitko/typeshed that referenced this pull request Jan 23, 2019
This fixes an error in Travis that seems to have been caused by python/mypy#4319.

The fix was taken from the stdlib/3.4/enum.pyi stub. Mypy no longer assumes
that classes whose metaclass is EnumMeta are subclasses of Enum, so we can't
bound the typevar on Enum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants