-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Remove QuerySet alias hacks via PEP 696 TypeVar defaults #2104
Conversation
ext/django_stubs_ext/aliases.py
Outdated
@@ -6,7 +6,7 @@ | |||
from django.utils.functional import _StrPromise as StrPromise | |||
|
|||
QuerySetAny = _QuerySetAny | |||
ValuesQuerySet = _QuerySet[_T, _Row] | |||
ValuesQuerySet = _QuerySet[_T, _Row] # TODO think about this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder for self.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and this works fine, even though query.pyi
now uses _Model
TypeVar instead of _T
in the QuerySet
class (_T
still exists).
It's best to keep these aliases as is for now. Since the dependency constraint between django-stubs
and django-stubs-ext
only goes one way, users may be running e.g. django-stubs-ext 5.0.1
and django-stubs 5.0.0
together.
We can simplify these aliases more a few releases down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think we can now (in the next PRs) get rid of WithAnnotations
and provide it as the third type var to QuerySet
.
d3cadd2
to
d8ecc95
Compare
After this change, we strictly require mypy >=1.10.0. Fortunately, older mypy versions will fail quite loudly. |
A 3rd typevar is not a good long-term solution for that. What we really need is
The type of class NumBooksAnnotation(Protocol):
num_books: int
QuerySet[Publisher, Publisher & NumBooksAnnotation] We currently emulate the We can't get rid of class NumBooksTypedDict(TypedDict):
num_books: int
def handle_publisher_with_numbooks(
publisher: WithAnnotations[Publisher, NumBooksTypedDict] # <-- (!) not a QuerySet
):
print(publisher.num_books)
for publisher in Publisher.objects.annotate(num_books=Count("book")):
handle_publisher_with_numbooks(publisher) |
- case: QuerySet_type_vars | ||
main: | | ||
from django.db.models.query import QuerySet | ||
from django.contrib.auth.models import User | ||
from django_stubs_ext import ValuesQuerySet | ||
|
||
a: QuerySet[User] | ||
reveal_type(a) # N: Revealed type is "django.db.models.query.QuerySet[django.contrib.auth.models.User, django.contrib.auth.models.User]" | ||
b: QuerySet[User, int] | ||
reveal_type(b) # N: Revealed type is "django.db.models.query.QuerySet[django.contrib.auth.models.User, builtins.int]" | ||
c: ValuesQuerySet[User, int] | ||
reveal_type(c) # N: Revealed type is "django.db.models.query.QuerySet[django.contrib.auth.models.User, builtins.int]" | ||
|
||
d: QuerySet[int] # E: Type argument "int" of "QuerySet" must be a subtype of "Model" [type-var] | ||
e: ValuesQuerySet[int] # E: Type argument "int" of "QuerySet" must be a subtype of "Model" [type-var] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed helpful to add a test for these behaviors. But not entirely sure it's worth it.
Anyway this is ready for review/merging from my point of view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, really great feature!
django-stubs/db/models/query.pyi
Outdated
|
||
QuerySet: TypeAlias = _QuerySet[_T, _T] | ||
# Obsolete aliases of QuerySet, for compatibility only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this compat alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, yes. ext/django_stubs_ext/aliases.py
still expects to import from django.db.models.query import _QuerySet, _QuerySetAny
. I'll need to think if we change aliases.py
without breaking compatibility.
It's really difficult to think about these, because the dependency constraint between django-stubs and django-stubs-ext only goes one way, users may be running e.g. django-stubs-ext 5.0.1
and django-stubs 5.0.0
together.
I wonder if we can make the version constraint strict django-stubs-ext==5.0.0
, or will that cause issues for package managers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw when adding review comments, please add it on the last relevant line of a change, rather than the first line, this way more of the diff context is visible.
Right now the message is missing the context that it's about _QuerySet
and _QuerySetAny
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an idea:
We can get rid of _QuerySetAny
after the above is merged. But the _QuerySet
alias must remain for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, thanks again!
The `QuerySet` class was previously named `_QuerySet` and had three aliases: `QuerySet`, `QuerySetAny` and `ValuesQuerySet`. These hacks were mainly needed to for the ergonomic single-parameter `QuerySet[Model]`, which expanded into `_QuerySet[Model, Model]` But now that mypy 1.10 implements PEP 696 to a fuller extent (Pyright also supports it), the 2nd type parameter can be a simple TypeVar that defaults to 1st type parameter.
d6e846a
to
d19d124
Compare
Created an "announcement" discussion for this deprecation as well: |
I have made things!
The
QuerySet
class was previously named_QuerySet
and had three aliases:QuerySet
,QuerySetAny
andValuesQuerySet
. These hacks were mainly needed to for the ergonomic single-parameterQuerySet[Model]
, which expanded into_QuerySet[Model, Model]
Now that mypy 1.10 implements PEP 696 Type Defaults for Type Parameters to a fuller extent (Pyright also supports it), the 2nd type parameter can be a simple TypeVar that defaults to 1st type parameter. All usages of
QuerySetAny
andValuesQuerySet
can now be replaced with simpleQuerySet
.typing-extensions
requrement to>=4.11.0
, since that's the one that addsTypeVar(default=)
parameter. https://github.com/python/typing_extensions/blob/main/CHANGELOG.md#release-4110-april-5-2024TODO:
Related issues