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

Remove QuerySet alias hacks via PEP 696 TypeVar defaults #2104

Merged
merged 5 commits into from
May 6, 2024

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Apr 30, 2024

I have made things!

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]

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 and ValuesQuerySet can now be replaced with simple QuerySet.

TODO:

  • Test behavior on mypy 1.9 and earlier
  • Update readme

Related issues

@intgr intgr changed the title Remove queryset hacks Remove QuerySet alias hacks via PEP 696 TypeVar defaults Apr 30, 2024
@@ -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!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reminder for self.

Copy link
Collaborator Author

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.

@intgr intgr marked this pull request as ready for review April 30, 2024 21:34
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.

Great! I think we can now (in the next PRs) get rid of WithAnnotations and provide it as the third type var to QuerySet.

@intgr
Copy link
Collaborator Author

intgr commented May 2, 2024

After this change, we strictly require mypy >=1.10.0. Fortunately, older mypy versions will fail quite loudly.

@intgr
Copy link
Collaborator Author

intgr commented May 3, 2024

Great! I think we can now (in the next PRs) get rid of WithAnnotations and provide it as the third type var to QuerySet.

A 3rd typevar is not a good long-term solution for that. What we really need is Intersection types.


The type of Publisher.objects.annotate(num_books=Count("book")) should boil down to:

class NumBooksAnnotation(Protocol):
    num_books: int

QuerySet[Publisher, Publisher & NumBooksAnnotation]

We currently emulate the & that with our own hand-grown Annotations type and TypedDict instead of Protocol.

We can't get rid of WithAnnotations with a QuerySet typevar anyway. It's not needed for just QuerySets, it's also needed when passing around Model instances containing annotations, e.g.

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)

Comment on lines +58 to +72
- 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]
Copy link
Collaborator Author

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.

@intgr
Copy link
Collaborator Author

intgr commented May 3, 2024

Anyway this is ready for review/merging from my point of view.

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.

Thanks a lot, really great feature!


QuerySet: TypeAlias = _QuerySet[_T, _T]
# Obsolete aliases of QuerySet, for compatibility only.
Copy link
Member

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?

Copy link
Collaborator Author

@intgr intgr May 3, 2024

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

ext/django_stubs_ext/aliases.py Outdated Show resolved Hide resolved
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.

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.
@intgr intgr added the release notes reminder User impact should be explicitly documented in release notes. label May 3, 2024
@intgr intgr merged commit 4a5b065 into typeddjango:master May 6, 2024
40 checks passed
@intgr intgr deleted the remove-queryset-hacks branch May 6, 2024 09:06
@flaeppe flaeppe removed the release notes reminder User impact should be explicitly documented in release notes. label May 27, 2024
@intgr
Copy link
Collaborator Author

intgr commented May 29, 2024

Created an "announcement" discussion for this deprecation as well:

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.

Error messages refer to private _QuerySet class
3 participants