Allow any reverse relation using ForeignObjectRel to be type checked #1451
Conversation
Currently, reverse relations created using just `ForeignObjectRel` aren't checked. A real-world example of this is `django-composite-foreignkey`. Reverse relations created by it aren't discovered by the mypy plugin.
|
Whoa there. You need to take a few steps back and explain what As far as I can tell, |
|
Yes, adding a test case with pure django objects would help a lot as well. |
Although Django does not truly support composite/multi-column foreign keys as support for it hasn't yet been added to the schema editor and migration subsystem.
I don't think it's considered a truly stable/documented API, but the fact that changes to are mentioned in the changelogs indicates that the Django developers are aware there are quite a few third-party packages out there relying on it. Making it unlikely to disappear suddenly or subject to intense changes. Some more googling reveals that
On top of that, the class has existed since Django 1.6, introduced here: django/django@266de5f All-in; I think it's worth supporting. It would allow everyone using The test I added demonstrates how to use |
| main: | | ||
| from app1.models import Model1, Model2 | ||
|
|
||
| reveal_type(Model2.model_1) # N: Revealed type is "django.db.models.fields.related.ForeignObject[Any, Any]" |
There was a problem hiding this comment.
If you also add a check for Model2().model_1, does it correctly show up as Model1 type?
There was a problem hiding this comment.
No, it gets typed as Any. I will have a look to see if I can fix this.
There was a problem hiding this comment.
I've pushed a fix and updated the tests. It type-checks correctly now.
Photonios
force-pushed
the
support-composite-foreign-key
branch
from
1048b5a to
77fe2cf
Compare
mypy_django_plugin/lib/fullnames.py
Outdated
| RELATED_FIELDS_CLASSES = { | ||
| FOREIGN_OBJECT_FULLNAME, | ||
| FOREIGN_KEY_FULLNAME, | ||
| ONETOONE_FIELD_FULLNAME, | ||
| MANYTOMANY_FIELD_FULLNAME, | ||
| } |
There was a problem hiding this comment.
| RELATED_FIELDS_CLASSES = { | |
| FOREIGN_OBJECT_FULLNAME, | |
| FOREIGN_KEY_FULLNAME, | |
| ONETOONE_FIELD_FULLNAME, | |
| MANYTOMANY_FIELD_FULLNAME, | |
| } | |
| RELATED_FIELDS_CLASSES = frozenset(( | |
| FOREIGN_OBJECT_FULLNAME, | |
| FOREIGN_KEY_FULLNAME, | |
| ONETOONE_FIELD_FULLNAME, | |
| MANYTOMANY_FIELD_FULLNAME, | |
| )) |
There was a problem hiding this comment.
Thanks! I've applied the suggestion.
| ( | ||
| FOREIGN_OBJECT_FULLNAME, | ||
| FOREIGN_KEY_FULLNAME, | ||
| ONETOONE_FIELD_FULLNAME, |
There was a problem hiding this comment.
Both ForiegnKey and OneToOneField are subclasses of ForeignObject, so these could be removed now.
ManyToManyField has a separate inheritance hierarchy though.
There was a problem hiding this comment.
Also this will additionally apply to GenericRelation, which is another subclass of ForeignObject. Seems like a good thing, but ideally we should add a test to make sure it works as expected.
Currently, reverse relations created using just
ForeignObjectRelaren't checked. A real-world example of this isdjango-composite-foreignkey. Reverse relations created by it aren't discovered by the mypy plugin.If anyone has any ideas on how to write a proper test for this, let me know.