Repository navigation
Fixes #1727: constructor after classmethod counts - #5501
VadimPushtaev wants to merge 5 commits into
Conversation
|
Possibly, another solution is to do what |
|
Also, I don't quite like |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! This is an important issue to fix, but there are some potential dangers. I believe the right fix should also fix #4575 The underlying problem is the same: processing of class method signatures happens too soon.
Also the current solution has the problem that it might not work well with the fine grained daemon (because class is not a "real" fine-grained dependency target). Probably, I would move this step to the patches after third pass (at least after forward refs are resolved) unless there are some other problems.
Also please note many mypy core devs are on vacation now, so we may not always respond in time. I am only leaving "large scale" comments now.
| self._arg: int = arg | ||
|
|
||
| reveal_type(C.create) # E: Revealed type is 'def (arg: builtins.int) -> __main__.C' | ||
| [builtins fixtures/classmethod.pyi] |
There was a problem hiding this comment.
I am not sure this test actually tests the situation enough. I think what you need to add is a reveal_type for cls and cls(<expected args>) in the method body, and as well check that cls(<unexpected args>) fails with a reasonable error.
Please be sure to include all the examples that appeared in the original issue. In addition please add at least tests that check your fix works correctly with the following:
- Fine-grained daemon
- Forward references
- Generic classes
- Overloaded methods (there are known issues, but few simple tests would help)
| functype = func.type | ||
| assert self.type | ||
| assert functype | ||
| assert isinstance(functype, CallableType) |
There was a problem hiding this comment.
Why do you need these asserts? Please add assert messages.
There was a problem hiding this comment.
That was basically to make mypy happy of this code. Fixed to make more sense.
|
Thanks for the review, I'll try to fix all the issues as soon as possible. |
Constructor defined after classmethod was considered to have no args in the classmethod. This commit fixes that bug.
b39eb0f to
d5ed26a
Compare
|
I believe that's all tests we reasonably need. |
|
Hmm is this now superseded by #5646? Thanks for getting the ball rolling with this anyway! |
|
Yes, I think now that we have a broader fix, this one is not needed. Closing in favor of #5646 @VadimPushtaev Thank you for working on this! |
This is a fix for #1727.
Instead of prepare classmethod definitions while analyzing class, let's postpone this until the class is fully analyzed.
dataclassmagic creates__init__after the class is already created, so they fix up all the classmethods. This is no longer needed.