Add more method signatures for _ValuesQuerySet #661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaelAquilina
commented
Jul 3, 2021
| query = MyUser.objects.values_list('name') | ||
| reveal_type(query.order_by("name").get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.distinct("name").get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.distinct().get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.all().get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.filter(age__gt=16).get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.exclude(age__lte=16).get()) # N: Revealed type is "Tuple[builtins.str]" | ||
| reveal_type(query.annotate(name_length=Length("name")).get()) # N: Revealed type is "Any" |
There was a problem hiding this comment.
might be worth checking this one is correct - I wasnt 100% sure about the approach for annotate
sobolevn
approved these changes
Jul 3, 2021
| @@ -154,6 +154,9 @@ class _ValuesQuerySet(Generic[_T, _Row], Collection[_Row], Reversible[_Row], Que | |||
| def distinct(self, *field_names: Any) -> _ValuesQuerySet[_T, _Row]: ... # type: ignore | |||
| def order_by(self, *field_names: Any) -> _ValuesQuerySet[_T, _Row]: ... # type: ignore | |||
| def all(self) -> _ValuesQuerySet[_T, _Row]: ... # type: ignore | |||
| def annotate(self, *args: Any, **kwargs: Any) -> _ValuesQuerySet[_T, Any]: ... | |||
There was a problem hiding this comment.
Please, see #398
We are working on fixing Any there
Do you have some free time to try this out? 🙂 |
I'd definitely want to give it a try at the very least :) is there anything in particular I should be aware of before diving in? |
syastrov
added a commit
to syastrov/django-stubs
that referenced
this pull request
Jul 7, 2021
This currently has the drawback that error messages display the internal type _QuerySet, with both type arguments. See also discussion on typeddjango#661 and typeddjango#608. Fixes typeddjango#635: QuerySet methods on Managers (like .all()) now return QuerySets rather than Managers. Address code review by @sobolevn.
sobolevn
pushed a commit
that referenced
this pull request
Jul 23, 2021
* QuerySet.annotate returns self-type. Attribute access falls back to Any. - QuerySets that have an annotated model do not report errors during .filter() when called with invalid fields. - QuerySets that have an annotated model return ordinary dict rather than TypedDict for .values() - QuerySets that have an annotated model return Any rather than typed Tuple for .values_list() * Fix .annotate so it reuses existing annotated types. Fixes error in typechecking Django testsuite. * Fix self-typecheck error * Fix flake8 * Fix case of .values/.values_list before .annotate. * Extra ignores for Django 2.2 tests (false positives due to tests assuming QuerySet.first() won't return None) Fix mypy self-check. * More tests + more precise typing in case annotate called before values_list. Cleanup tests. * Test and fix annotate in combination with values/values_list with no params. * Remove line that does nothing :) * Formatting fixes * Address code review * Fix quoting in tests after mypy changed things * Use Final * Use typing_extensions.Final * Fixes after ValuesQuerySet -> _ValuesQuerySet refactor. Still not passing tests yet. * Fix inheritance of _ValuesQuerySet and remove unneeded type ignores. This allows the test "annotate_values_or_values_list_before_or_after_annotate_broadens_type" to pass. * Make it possible to annotate user code with "annotated models", using PEP 583 Annotated type. * Add docs * Make QuerySet[_T] an external alias to _QuerySet[_T, _T]. This currently has the drawback that error messages display the internal type _QuerySet, with both type arguments. See also discussion on #661 and #608. Fixes #635: QuerySet methods on Managers (like .all()) now return QuerySets rather than Managers. Address code review by @sobolevn. * Support passing TypedDicts to WithAnnotations * Add an example of an error to README regarding WithAnnotations + TypedDict. * Fix runtime behavior of ValuesQuerySet alias (you can't extend Any, for example). Fix some edge case with from_queryset after QuerySet changed to be an alias to _QuerySet. Can't make a minimal test case as this only occurred on a large internal codebase. * Fix issue when using from_queryset in some cases when having an argument with a type annotation on the QuerySet. The mypy docstring on anal_type says not to call defer() after it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have made things!
Follow up from #657 - I found more cases in my codebase where method signatures where incorrectly missing.
Looking at #608 it seems like this issue has been discussed at length already about why this is an issue.
My initial thoughts was that maybe there was a way of having:
_QuerySet[_T, _Row]typeQuerySet[_T]which is equivalent to_QuerySet[_T, _T]_QuerySet[_T, _Row]as appropriate.But based on the conversation you've had it seems like this might be harder than I assume it is?
The downside of keeping it in the current way is that we essentially need to redefine all
QuerySetmethod signatures twice: Once forQuerySetand once for_ValuesQuerySet