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 generics from QuerySet type #794

Closed
wants to merge 2 commits into from
Closed

Conversation

jerch
Copy link

@jerch jerch commented Dec 28, 2021

See #704 - currently it is not possible to deal with QuerySet class obj in python without getting a generics alert. By removing the [_T, _T] it works again without complaints.

Related issues

Closes #704.

I have no idea about possible side effects of this change, so please someone with more knowledge about the typing internals of django-stubs would have to review this change.

@sobolevn
Copy link
Member

Maybe like this?

class QuerySet(_QuerySet[_T, _T]): ...

Or:

class _QuerySetAlias(_QuerySet[_T, _T]): ...

QuerySet = _QuerySetAlias

@jerch
Copy link
Author

jerch commented Dec 28, 2021

@sobolevn Yes, trying your first variant (as it looks nicer to me). Still I wonder if the queryset base class has a model base class connection at all (have not looked up the django source). In the end it does not matter as long as the interfaces with types are announced correctly.

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! We still need a test case for #704

@jerch
Copy link
Author

jerch commented Dec 28, 2021

@sobolevn Are those explicit tests against _QuerySet[XY, XY] needed or can they be replaced by QuerySet[XY]? Would that lose test/type information?

@sobolevn
Copy link
Member

Are those explicit tests against _QuerySet[XY, XY] needed

Yes, we need them to test our internal representation of QuerySet, which is needed for things like .annotate() to work.

@jerch
Copy link
Author

jerch commented Dec 29, 2021

@sobolevn Not sure if I can help you with the test cases. I am getting tons of errors, if I try to run pytest locally in a venv with python 3.10 (bootstrapped similar to the CI setup), like these:

/home/jerch/Dokumente/github/django-stubs/tests/typecheck/test_config.yml:5: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ../../home/jerch/Dokumente/github/django-stubs/django-stubs/db/models/deletion.pyi:12: error: Bad number of arguments for type alias, expected: 2, given: 1 (diff)

<here like 30 more errors of the same type in other places>

E     main:3: note: Revealed type is "builtins.int*" (diff)
E     main:4: note: Revealed type is "django.contrib.auth.models.User*" (diff)
E     main:5: note: Revealed type is "django.db.models.manager.Manager[myapp.models.MyModel]" (diff)
E     myapp/models:6: note: Revealed type is "django.contrib.auth.models.User*" (diff)
E   Expected:
E     main:3: note: Revealed type is "builtins.int*" (diff)
E     main:4: note: Revealed type is "django.contrib.auth.models.User*" (diff)
E     main:5: note: Revealed type is "django.db.models.manager.Manager[myapp.models.MyModel]" (diff)
E     myapp/models:6: note: Revealed type is "django.contrib.auth.models.User*" (diff)
E   Alignment of first line difference:
E     E: main:3: note: Revealed type is "builtins.int*"...
E     A: ../../home/jerch/Dokumente/github/django-stubs/django-stubs/db/models/de...
E        ^

@intgr
Copy link
Collaborator

intgr commented Jan 3, 2022

I tried this change and it causes new errors in my code. For example:

            MyModel.objects.annotate(
                last_status=Subquery(
                    OtherModel.objects.values("last_status")
                )

error: Argument 1 to "Subquery" has incompatible type "_QuerySet[OtherModel, TypedDict({'last_status': str})]"; expected "QuerySet[Any]"

Because _QuerySet and QuerySet are now distinct types, _QuerySet cannot be converted to its subclass QuerySet.

I think some changes are needed in the plugin code as well.

@jerch
Copy link
Author

jerch commented Jan 3, 2022

@intgr Thats the reason why I asked if QuerySet[XY] could replace _QuerySet[XY, XY]. But your use case already answers that as a clear no (see the different types in _QuerySet[OtherModel, TypedDict({'last_status': str})]. Sadly I cannot get the test cases working locally, thus cannot do much to get things fixed.

@PIG208 PIG208 mentioned this pull request Oct 12, 2022
12 tasks
@intgr intgr added the stale Pull requests that have been out of date or unmergeable for a long time. label Jan 10, 2023
@intgr
Copy link
Collaborator

intgr commented Jan 10, 2023

This seems redundant now that #794 has been merged?

Feel free to re-open if I'm missing something.

@intgr intgr closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Pull requests that have been out of date or unmergeable for a long time.
Development

Successfully merging this pull request may close these issues.

[Question] What is an alternative to isinstance(obj, QuerySet)?
3 participants