-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Support six.with_metaclass #3364
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
mypy/semanal.py
Outdated
@@ -953,6 +955,28 @@ def analyze_base_classes(self, defn: ClassDef) -> None: | |||
if defn.info.is_enum and defn.type_vars: | |||
self.fail("Enum class cannot be generic", defn) | |||
|
|||
def get_base_type_exprs(self, defn: ClassDef) -> List[Expression]: |
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.
Document that this may modify defn.metaclass
-- maybe also include this information in the method name.
mypy/semanal.py
Outdated
and all(kind == ARG_POS for kind in base_expr.arg_kinds)): | ||
metaclass = base_expr.args[0] | ||
if isinstance(metaclass, NameExpr): | ||
defn.metaclass = metaclass.name |
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 be a fully qualified name? Anyway, test code like this:
from m import M
import six
class C(six.with_metaclass(M)): ...
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 just copied this from analyze_metaclass()
. Added a test, it seems to work.
test-data/unit/check-classes.test
Outdated
class C2(C1, six.with_metaclass(M)): pass # E: Invalid base class | ||
class C3(six.with_metaclass(A)): pass # E: Metaclasses not inheriting from 'type' are not supported | ||
class C4(six.with_metaclass(M), metaclass=M): pass # E: Invalid base class | ||
|
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:
- Dynamic metaclass with
six.with_metaclass
, such assix.with_metaclass(f())
. - Member expression as metaclass, such as
six.with_metaclass(m.M)
. - Metaclass from-imported from another module (
from m import M
); also mentioned above.
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.
All added.
@JukkaL: Do you have time for another review? |
mypy/semanal.py
Outdated
@@ -902,7 +902,9 @@ def analyze_base_classes(self, defn: ClassDef) -> None: | |||
|
|||
base_types = [] # type: List[Instance] | |||
info = defn.info | |||
for base_expr in defn.base_type_exprs: | |||
|
|||
base_type_exprs = self.check_with_metaclass(defn) |
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 is not a right place for this check. It should be rather processed before (or in) clean_up_bases_and_infer_type_variables
, this is why you see problems with Generic
in #3366.
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! Much better now. I also change the API for check_with_metaclass() a bit, but it does exactly the same thing (except returns both values). Please review again.
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 to me now.
This allows Mypy to understand six.with_metaclass: python/mypy#3364
Fixes #1764
This doesn't add support for
@six.add_metaclass(M)
-- if that's required we'll add it later (we should open a separate issue for that).