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.

Sign up for GitHub

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

Jump to bottom

Introduce QuerySetAny type for QuerySet isinstance checks #1199

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Introduce QuerySetAny type for QuerySet isinstance checks #1199

merged 1 commit into from
Nov 3, 2022

Conversation

PIG208
Copy link
Contributor

@PIG208 PIG208 commented Oct 21, 2022 •
edited
Loading

This also re-export QuerySetAlias for external access to generic _QuerySet. We use this in our test cases as well. The approach taken here is making QuerySet a subclass of _QuerySet[_T, _T], and make _QuerySetAlias a type alias of _QuerySet[_T, _T].

_QuerySetAlias[_T] should be used internally whenever the type parameter is needed, replacing QuerySet[_T] entirely within the stubs; both QuerySet and _QuerySetAlias are accepted in use cases where the type parameter is not needed.

I have made things!

Related issues

@PIG208
Copy link
Contributor Author

PIG208 commented Oct 21, 2022

One concern is that we are forcing end-users to use QuerySetAlias in type annotations.

@PIG208
Copy link
Contributor Author

PIG208 commented Oct 21, 2022

TODO: Add test cases for isinstance checks with QuerySet

@intgr
Copy link
Collaborator

intgr commented Oct 21, 2022

If this gets merged, all downstream users using QuerySet[MyModel] need to rewrite them as QuerySetAlias[MyModel]? That's a pretty high cost, in my codebase there are 230 instances of QuerySet[] hints.

I understand this is to fix isinstance(x, QuerySet), which I have not needed anywhere so far.

I agree the latter issue deserves to be fixed, but before inflicting such a large migration on users, we should be quite sure there are no better alternatives. Maybe there's something that can be done with mypy plugins to avoid this?

@sobolevn
Copy link
Member

Yeah, I am also quite worried about this. This seems very high-risk.

@adamchainz
Copy link
Contributor

I am also worried about making such a change. We should maintain QuerySet[MyModel] as a priority. Django was even changed to support it: django/django@578c03b276e . It wouldn't be possible to remove that without a whole deprecation cycle anyway.

@PIG208
Copy link
Contributor Author

PIG208 commented Oct 21, 2022 •
edited
Loading

I think we can also do this the other way around, by making QuerySetAlias non-generic, and continuing using QuerySet[_T].

@PIG208 PIG208 force-pushed the generics branch 2 times, most recently from 94b89bd to 5c0ef2c Compare October 21, 2022 19:48
@PIG208
Copy link
Contributor Author

PIG208 commented Oct 21, 2022 •
edited
Loading

The PR is updated.

The approach taken here is making _QuerySetAlias a subclass of _QuerySet[_T, _T] dedicated for isinstance checks, and leave QuerySet unchanged as the type alias of _QuerySet[_T, _T].

A slight disadvantage of this approach is that QuerySet is not a subclass of QuerySetAlias, and thus using QuerySetAlias in type annotation is generally incorrect. But the fix should suffice for the isinstance check use case. This is noted in the added documentation.

@PIG208 PIG208 force-pushed the generics branch 2 times, most recently from 4979aa1 to fedc759 Compare October 21, 2022 20:33
sobolevn
sobolevn reviewed Oct 22, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
sobolevn
sobolevn reviewed Oct 22, 2022
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This way we can remove a new class. It might affect some cases like invariant lists.

(Please, also add a test for list[QuerySet].append(query_set_instance_obj) case).

django-stubs/db/models/query.pyi Outdated Show resolved Hide resolved
sobolevn
sobolevn approved these changes Oct 27, 2022
View reviewed changes
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.

Thank you! Looks great now!

I will let others to provide their feedback before merging.

@PIG208
Copy link
Contributor Author

PIG208 commented Oct 27, 2022

Renamed the test case to test_querysetany.

adamchainz
adamchainz approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

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

LGTM, I think the new name is suitable.

README.md Outdated Show resolved Hide resolved
@PIG208
Add QuerySetAny as a non-generic variant of QuerySet.
d5119af 
This also re-export `QuerySetAny` for external access to
nongeneric QuerySet.

The approach taken here is making `_QuerySetAny` an alias
of `_QuerySet[_T, _T]` dedicated for isinstance checks, and
leave `QuerySet` unchanged as the type alias of `_QuerySet[_T, _T]`.

Fixes typeddjango#704.

Signed-off-by: Zixuan James Li <p359101898@gmail.com>
sobolevn
sobolevn reviewed Oct 27, 2022
View reviewed changes
README.md
variant of `QuerySet` suitable for runtime type checking:

```python
from django_stubs_ext import QuerySetAny
Copy link
Member

Choose a reason for hiding this comment

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

Question for other PRs: do we have other types to take care of?

@PIG208
Copy link
Contributor Author

PIG208 commented Nov 3, 2022

I think this is ready to merge.

sobolevn
sobolevn approved these changes Nov 3, 2022
View reviewed changes
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.

Ok, QuerySetAny also seems like a good name!

@sobolevn sobolevn merged commit e88f942 into typeddjango:master Nov 3, 2022
@PIG208 PIG208 deleted the generics branch November 3, 2022 19:54
@intgr intgr changed the title Make QuerySet non-generic. Introduce QuerySetAny type for QuerySet isinstance checks Dec 8, 2022
@PIG208 PIG208 mentioned this pull request Mar 16, 2023
Merged
12 tasks
@intgr
Copy link
Collaborator

intgr commented May 29, 2024

QuerySetAny is now deprecated and can be replaced with a simple isinstance(x, QuerySet) check. More details here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intgr intgr intgr left review comments

@adamchainz adamchainz adamchainz approved these changes

@sobolevn sobolevn sobolevn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Question] What is an alternative to isinstance(obj, QuerySet)?
4 participants
@PIG208 @intgr @sobolevn @adamchainz