Skip to content

Conversation

w0rp
Copy link
Contributor

@w0rp w0rp commented Nov 23, 2022

I have made things!

I'm creating this pull request now primarily because I need some help implementing this. I am updating the Manager class to hold the QuerySet class for the manager as a generic type parameter, as per the issue.

I have gotten some parts to work, and the types should work for type checkers without plugin support. Figuring out how to get the mypy plugin to work however is absolutely maddening. There are several hooks where a manager is changed in one place, and then again in a totally different place, and then maybe again in another, and I am having difficulty figuring out exactly how to change the Manager types so they have the right type parameters in the plugin code.

If I can fix the plugin and test cases, this patch will improve type information for mypy and other type checkers that don't have plugin support like Pyright.

Related issues

Closes #863

@flaeppe
Copy link
Member

flaeppe commented Sep 18, 2023

This is late, I know, but I have one question here, while I'm aware that this is something that isn't really enforced today either; If a manager is generic over a model and a QuerySet, while a QuerySet is generic over a model, how do we then get insurance that it's the same model for both Manager and QuerySet?

To some extent I do agree that a manager should/could be generic over a model and queryset though.

And the plugin yes, it's quite the spaghetti while it also have to do a lot of work puzzling together managers with querysets in different and multiple stages of mypy's processing.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. I feel like we should wait a few releases with it, because the last one was quite complex for our users.

Plus, I want to be sure that we have enough benefits in doing this.

@Viicos
Copy link
Contributor

Viicos commented Dec 22, 2023

how do we then get insurance that it's the same model for both Manager and QuerySet?

afaik you can't. Actually the current _QS type variable is invalid (@w0rp currently added a type ignore). This is a use case that probably requires python/typing#548. I'll explore the path of this PR a bit more and see if I can find something suitable.

@finlayacourt
Copy link

Now that we have default types would it be possible to impliment this change in a non-breaking way?

_T = TypeVar("_T", bound=Model, covariant=True)
_QS = TypeVar("_QS", bound=QuerySet[Any], covariant=True, default=QuerySet[_T])

class BaseManager(Generic[_T, _QS]):
    def get_queryset(self) -> _QS: ...

The default also ensures that the queryset is bound by the correct model unless the type param is explicitly overridden

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.

Making QuerySet classes part of the type of Manager classes
5 participants