-
-
Notifications
You must be signed in to change notification settings - Fork 521
Only populate a model argument for generic managers #1683
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
b03efe1 to
60b96e6
Compare
sobolevn
left a comment
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.
This is more correct! Thanks!
| continue | ||
|
|
||
| manager_type = Instance(manager_info, [Instance(self.model_classdef.info, [])]) | ||
| if manager_info.is_generic(): |
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.
Maybe refactor into helper?
| m_generic_from_qs_generic = ManagerGenericFromQuerySetGeneric() | ||
| qs_populated_as_manager = QuerySetPopulated.as_manager() | ||
| qs_generic_as_manager = QuerySetGeneric["MyModel"].as_manager() |
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 I found a mypy bug here, but I'm not sure. But the get_dynamic_class_hook isn't called for QuerySetGeneric["MyModel"].as_manager() due to ["MyModel"].
If I remove ["MyModel"] it is triggered. But that means the hook isn't compatible with a classmethod on a generic class.
I have no idea if it's unreasonable to expect it to be triggered, it just feels like a bug that adding type parameters affects the triggering..
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.
Please, report it anyways :)
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.
(and tag me, since I don't watch all issues)
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 was already discovered, some time ago. That time from django stubs as well: python/mypy#8359
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.
We can add a test-case with xfail for now. After mypy@1.6.0 we will just remove xfail and move on.
|
Hey, just chiming in to say I really appreciate all the work you all do on the plugin code to make it more reliable. Keep up the good work, would have loved to help you a bit but it's holidays season for me currently ahah. Anyway, thanks! |
|
I believe this was done in #2314 |
I have made things!
Change the plugin so that non generic managers (like below) won't get an implicit model argument
Previously these(
MyModel.objects) were assigned a type like:MyManager["MyModel"]TODO:
Related issues
ClassVars #1672 (comment)