Add typing to metaclass methods
#1678
Conversation
DanielNoord
added
the
Maintenance
| def declared_metaclass(self, context=None): | ||
| def declared_metaclass( | ||
| self, context: InferenceContext | None = None | ||
| ) -> SuccessfulInferenceResult | None: |
There was a problem hiding this comment.
Or do we want to assert the infer() to make sure we only return certain types? It doesn't make a lot of sense for a metaclass to be an UnboundMethod (for example), which it could be with the current typing and lack of assertions.
There was a problem hiding this comment.
From what I can tell or at least in the astroid tests. Only nodes.ClassDef | None is ever returned. I.e. it could actually make sense to restrict the return type here. Especially since metaclass could be called independently of infer.
I'm not sure how we can add an assert here though without changing the return generator expression. Might be better to use type: ignore or cast instead.
There was a problem hiding this comment.
My idea was to change the next call in the return statement to only allow ClassDef to pass through. That should work right?
There was a problem hiding this comment.
Ideally we should get a warning / error if somehow something else would be returned. Otherwise we might mask an issue that is entirely preventable.
I was also thinking about wether it should be ClassDef or NodeNG. Currently, I would lean towards NodeNG. It might not be as precise, but users can do a simple isinstance check afterwards. Or do you think ClassDef would be better?
There was a problem hiding this comment.
Ideally we should get a warning / error if somehow something else would be returned. Otherwise we might mask an issue that is entirely preventable.
I don't really have any idea on how to allow this. Do you want to log something when this occurs?
I was also thinking about wether it should be
ClassDeforNodeNG. Currently, I would lean towardsNodeNG. It might not be as precise, but users can do a simpleisinstancecheck afterwards. Or do you thinkClassDefwould be better?
Yeah I think that's better. Especially considering plugins might also set meta classes and might not be as strict as we are.
There was a problem hiding this comment.
I have added the NodeNG check.
Pull Request Test Coverage Report for Build 2587679823
💛 - Coveralls |
|
@cdce8p Gentle ping as this is blocking some other typing work |
|
Merging as I think this is still a good change! |
Regression from pylint-dev#1678
Regression from #1678
Steps
Description
Type of Changes
Related Issue