Skip to content

Add objects attribute to Token - #644

Merged
sobolevn merged 1 commit into
masterfrom
sobolevn-patch-9
Jul 26, 2024
Merged

Add objects attribute to Token#644
sobolevn merged 1 commit into
masterfrom
sobolevn-patch-9

Conversation

@sobolevn

Copy link
Copy Markdown
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

sobolevn commented Jul 26, 2024

Copy link
Copy Markdown
Member Author

@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

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

@flaeppe

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

flaeppe commented Jul 26, 2024

Copy link
Copy Markdown
Member

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

@sobolevn

sobolevn commented Jul 26, 2024

Copy link
Copy Markdown
Member Author

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