search

typeddjango / djangorestframework-stubs

PEP-484 stubs for django-rest-framework
MIT License
453 stars 117 forks source link

Add `objects` attribute to `Token` #644

Closed sobolevn closed 3 months ago

sobolevn commented 3 months ago

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

sobolevn commented 3 months ago

@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 commented 3 months ago

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

flaeppe commented 3 months ago

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 commented 3 months ago

I've double checked, it's indeed from https://github.com/typeddjango/django-stubs/pull/2276

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

flaeppe commented 3 months ago

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

The thing is that since https://github.com/typeddjango/django-stubs/pull/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 commented 3 months ago

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 commented 3 months ago

@sobolevn I've opened https://github.com/typeddjango/django-stubs/pull/2280

sobolevn commented 3 months ago

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