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

Options for cooperation with django-stubs #207

Open
intgr opened this issue Oct 31, 2023 · 18 comments
Open

Options for cooperation with django-stubs #207

intgr opened this issue Oct 31, 2023 · 18 comments

Comments

@intgr
Copy link

intgr commented Oct 31, 2023

Hi! I'm one of the maintainers of the django-stubs project.

I sense that there is significant duplication of effort between django-stubs and django-types. Our pace of development has picked up recently, but still we're struggling to cover the Django API surface. If we joined forces, we could get more done.

If you're open to that, we need an understanding about the differences between the projects, here's what I'm aware of:

  1. I think the initial reason for forking was that django-stubs requried mypy? As of django-stubs version 4.2.6, mypy is no longer a required dependency.
  2. django-stubs relies on our mypy plugin magic for inferring Models/QuerySets. django-types instead documents how users should annotate Django's magic attributes on Model classes.
  3. There are differences in how mypy and pyright interpret type stubs. Hopefully not mutually incompatible, other projects (e.g. typeshed) work fine with multiple type checkers. We have already seen some contributions from pyright users.
  4. Discussion #147 points out that django-stubs uses _pyi_private_... attributes in Model fields for our magic. I wouldn't think their existence interferes when using pyright though?

(1) is solved and the rest don't look insurmountable.

Some relevant discussion:

Ping @sobolevn for visibility.

@intgr
Copy link
Author

intgr commented Oct 31, 2023

I found this in Django wiki (GSoC 2023): https://code.djangoproject.com/wiki/SummerOfCode2023#Mergingdjango-typesbackintodjango-stubs

I'm wondering if you agree with this characterisation:

There is a spin-off project from django-stubs called django-types. The original intention here was for it to be merged back into django-stubs, but that's never happened.

django-types removes the need to use any mypy plugins, making it possible to use type checkers other than mypy, such as Pyrite, Pyre, etc.

django-types is not actively maintained, however, and having competing stubs implementations means neither is as strong as could be.

There is a small project in extracting the work from django-types and creating a PR for django-stubs allowing it to be merged back in, and effort to be concentrated on the single implementation.

@intgr
Copy link
Author

intgr commented Oct 31, 2023

Here's also a description about django-stubs/django-types differences:

@sbdchd
Copy link
Owner

sbdchd commented Oct 31, 2023

I found this in Django wiki (GSoC 2023): https://code.djangoproject.com/wiki/SummerOfCode2023#Mergingdjango-typesbackintodjango-stubs

I'm wondering if you agree with this characterisation:

There is a spin-off project from django-stubs called django-types. The original intention here was for it to be merged back into django-stubs, but that's never happened.
django-types removes the need to use any mypy plugins, making it possible to use type checkers other than mypy, such as Pyrite, Pyre, etc.
django-types is not actively maintained, however, and having competing stubs implementations means neither is as strong as could be.
There is a small project in extracting the work from django-types and creating a PR for django-stubs allowing it to be merged back in, and effort to be concentrated on the single implementation.

Tbh it was a while ago but I think the original intention was to get autocomplete to work in VSCode to make my job (and coworkers) working in the Django codebase easier. I was cool with maintaining the project solo since we didn’t use that many Django features, a small surface area. I think we only used Django as a JSON API so the admin and related tools weren’t that important.

So once the models and query sets were working I considered the project good enough and would occasionally add some more types if autocomplete wasn’t working in some cases.

Ideally I was hoping for Django to incorporate types but I think they said no a while back.

I no longer work for that company or work in Django regularly so this project is more of a, “whenever I check GitHub and have the time to merge a PR”

I’m open to merging the projects, I think there would need to be parity since people are using this project as a dependency

@Viicos
Copy link

Viicos commented Dec 4, 2023

One key difference between the two libraries can be found in related fields: django-types is similar to django-stubs for RelatedField:

  • django-types signature: class RelatedField(FieldCacheMixin, Generic[_ST, _GT], Field[_ST, _GT])
  • django-stubs signature: class RelatedField(FieldCacheMixin, Field[_ST, _GT])

But starting at ForeignObject and subclasses, signature differs:

  • django-types: class ForeignObject(Generic[_M], RelatedField[_M, _M]) (where _M = TypeVar("_M", bound=Model | None))
  • django-stubs: class ForeignObject(RelatedField[_ST, _GT]).

With django-types annotating a ForeignKey for example is pretty straightforward:

class Model(models.Model):

    article = models.ForeignKey["Article"]("Article", on_delete=models.CASCADE)

Model().article  # revealed type is Article

but this is unfortunately not possible with django-stubs, as you need to parametrize both the set and get type:

class Model(models.Model):

    article = models.ForeignKey["Article", "Article"]("Article", on_delete=models.CASCADE)

Model().article  # revealed type is Article, revealed type is Any if using the first code snippet

Is there any reason for django-stubs to have both a distinct set and get type for forein keys & co?

Edit: forgot that you could set Model().article = <a_pk>, hence the need for the two type vars :/

@mordae
Copy link

mordae commented Dec 13, 2023

Edit: forgot that you could set Model().article = <a_pk>, hence the need for the two type vars :/

Which can be practically worked around by setting article_id instead.

@tylerlaprade
Copy link

FWIW, when I use django-types, I don't even need to annotate the type of ForeignKey. Pyright just knows the type out of the box. In django-stubs, it ends up unknown.

@Viicos
Copy link

Viicos commented Dec 13, 2023

FWIW, when I use django-types, I don't even need to annotate the type of ForeignKey. Pyright just knows the type out of the box. In django-stubs, it ends up unknown.

Could you be more specific? Is it unknown when accessing the foreign key field from an instance? From the class? By unkown, do you mean the actual Unknown type or Any? Are you using a string reference to the model, or a direct reference to the model?

@tylerlaprade
Copy link

That's right, it's Unknown in Pyright when I try to directly reference it.
image
image

The same code when not using django-stubs and only using django-types:
image
image

This means that with django-stubs, I only get type-safety for the first level of accessing attributes - if I did fx_rate.foo, that would be an error. But I don't get type-safety beyond that - I can do fx_rate.company.foo without any errors, while django-types catches that.

@sobolevn
Copy link

That's exactly the point of this issue: to make django-stubs and pyright compatible.

@intgr
Copy link
Author

intgr commented Dec 14, 2023

Yep. I have looked into the tricks that django-types uses to get better type inference for model fields without the plugin. It should be possible to achieve the same results one way or another, without degrading experience with mypy. But I haven't had the time to continue with that work.

@tylerlaprade
Copy link

@sobolevn, I was clarifying since @Viicos's message implied that one would need to manually annotate even with django-types, which is not the case (at least in Pyright, I'm not sure about Mypy).

@Viicos
Copy link

Viicos commented Dec 14, 2023

@tylerlaprade Thanks for the clarification. Both mypy and pyright should support the provided snippet with django-types. But this only works when using a direct reference to the model (instead of the string version {app_label}.{model_name}/{model_name}).

Actually django-stubs could support this use case as well by defining:

class ForeignKey(ForeignObject[_ST, _GT]):
    # As an extra overload:
    def __init__(
        self,
        to: type[_GT],
        ...
    )

But the set type can't really be parametrized this way, and this doesn't take into account nullability anyway.

FYI, I'm working on an alternative solution to support dango-stubs natively without plugins and without modifications to the actual type variables used. I will share more info soon.

@jereze
Copy link

jereze commented Dec 14, 2023

I apologize for appearing in the middle of the discussion, but this might be related. I was recently searching for how to get type hinting for ForeignKey fields using string references in Vscode (which uses django-types), and I got some advice in this discussion.

@Viicos
Copy link

Viicos commented Dec 14, 2023

Is there any reason for django-stubs to have both a distinct set and get type for forein keys & co?

Edit: forgot that you could set Model().article = <a_pk>, hence the need for the two type vars :/

After further investigation and looking at the descriptor implementation, you can't set the foreign field by it's primary key, so I don't think having two type vars for foreign fields is necessary? Or at least make both the set and get type the same typevar.

Or am I missing something?

Edit: It seems to be related with Combinable, which can be used on the set type. This seems to be the reason?

@Viicos
Copy link

Viicos commented Dec 17, 2023

FYI, I'm working on an alternative solution to support dango-stubs natively without plugins and without modifications to the actual type variables used. I will share more info soon.

You can find more info about this in the following article: https://viicos.github.io/posts/an-alternative-to-the-django-mypy-plugin/

django-autotyping is available here: https://github.com/Viicos/django-autotyping

Would love getting some feedback on this.

Yep. I have looked into the tricks that django-types uses to get better type inference for model fields without the plugin. It should be possible to achieve the same results one way or another, without degrading experience with mypy. But I haven't had the time to continue with that work.

I've been looking at this when implementing django-autotyping. I will come back here later with my findings on this subject.

@tylerlaprade
Copy link

@Viicos, interesting idea, but I don't understand why it's necessary. Pyright already knows the underlying type of a ForeignKey without any annotations when using django-types.

@Viicos
Copy link

Viicos commented Dec 18, 2023

I don't understand why it's necessary

Well as I said here, it doesn't support string references to models, which are pretty common (I would even recommend using them over direct reference to the actual related class for different reasons). The article also explains a bit about this.

Apart from support for ForeignKey, the tool will support a lot of features from the mypy plugin:

  • Signature for model __init__: your IDE will actually show what are the available fields when typing MyModel(...
  • Signatures for queryset/manager methods (get, filter, create, etc). Here as well, you will get IDE support, with a custom signature for the current model being queried.

@tylerlaprade
Copy link

Ah I see, thanks for clarifying!

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

No branches or pull requests

7 participants