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

Add objects attribute to Token #644

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Add objects attribute to Token #644

merged 1 commit into from
Jul 26, 2024

Conversation

sobolevn
Copy link
Member

Yes, I remember now that adding explicit objects helps with pyright

@sobolevn sobolevn requested a review from flaeppe July 26, 2024 06:01
@sobolevn
Copy link
Member Author

sobolevn commented Jul 26, 2024

@flaeppe one more question. I really hope that this change won't affect user-defined models. (I think we would have lots of tests failing then, but still. Since many models in the wild don't have explicit managers defined).

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

I'm not sure, was thinking about bisecting from #643 and see what commit I run into

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

Oh, right, it was green 11 hours ago in https://github.com/typeddjango/djangorestframework-stubs/actions/runs/10099849613

Resolved https://github.com/typeddjango/django-stubs to commit d747285f9597eb0942cd2e5049ed2cec6a0ac90a

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

I've double checked, it's indeed from typeddjango/django-stubs#2276

It looks like a repro case should be trivial in this case? Gonna see if I can figure it out

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

I don't have a repro other than this, but I think I figured it out:

The thing is that since typeddjango/django-stubs#2276 now goes through the mro it conflicts with the
mypy_django_plugin.transformers.models.MetaclassAdjustments that removes .objects from models.Model. This in itself isn't a problem as long as we (or mypy) can enforce an ordering such that we always process models.Model before anything else. This nearly happens, it just seems to be that the TypeInfo for models.Model isn't fully populated at that point and nothing happens. Later on the lookup via the mro finds objects on models.Model, but a while afterwards objects is instead removed via MetaclassAdjustments and we get the error.

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

django-stubs needs to provide a fix for this. This PR shouldn't be necessary when running the plugin(though it is for other cases).

I'm leaning towards reverting the plugin code that removes Model.objects. Most users seem to run with default behaviour of having an objects manager, we have multiple issues with different angles in django-stubs regarding this. And to follow Django's behaviour for other type checkers we'd probably want to declare e.g. objects as an abstract attribute. But there's currently no support for abstract attributes in Python

@flaeppe
Copy link
Member

flaeppe commented Jul 26, 2024

@sobolevn I've opened typeddjango/django-stubs#2280

@sobolevn
Copy link
Member Author

sobolevn commented Jul 26, 2024

This PR is still fine, because it will also help pyright users, which don't use our plugin. And it unblocks our CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants