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

Support returning the correct values for the different QuerySet methods when using .values() and .values_list(). #33

Merged
merged 11 commits into from
Mar 10, 2019

Conversation

syastrov
Copy link
Contributor

No description provided.

@syastrov syastrov force-pushed the queryset_values_and_values_list branch from 9bde821 to 8892414 Compare February 28, 2019 07:27
@mkurnikov
Copy link
Member

It's not clear to me, how all of that works. Could you add tests for other methods of QuerySet too? I'd split it into a number of tests:

  1. first(), earliest(), __getitem__, etc. for plain QuerySet.
  2. Same but for values() (this should return TypedDict some time in the future)
  3. Same but for values_list().

in_bulk may or may not be as part of every test, add it where it's applicable.

@syastrov
Copy link
Contributor Author

I have added a lot more tests, and in the process, caught that I had missed updating the .iterator method.

I haven't tested all of the queryset methods that I changed at the moment, as it's quite tedious to create all the cases. If I get some more time, I will add some more.

The annoying thing is that you cannot tell mypy that the default type for _Row is _T. It would make it a lot more user-friendly for annotating your own code.
Unfortunately, right now I believe you would have to e.g. write QuerySet[MyModel, MyModel] to avoid having mypy type it as QuerySet[MyModel, Any].
They have an open issue for that: python/mypy#4236

@mkurnikov
Copy link
Member

I haven't tested all of the queryset methods that I changed at the moment, as it's quite tedious to create all the cases. If I get some more time, I will add some more.

Sorry, I was wrong/imprecise in my comment, testing all the methods is indeed very tedious and pretty redundant. It should be enough to select just one of the group with the same number of generics present in the signature and same return type, like one of

    def none(self) -> QuerySet[_T]: ...
    def all(self) -> QuerySet[_T]: ...
    def filter(self, *args: Any, **kwargs: Any) -> QuerySet[_T]: ...
    def exclude(self, *args: Any, **kwargs: Any) -> QuerySet[_T]: ...
    def complex_filter(self, filter_obj: Any) -> QuerySet[_T]: ...
    def count(self) -> int: ...
    def union(self, *other_qs: Any, all: bool = ...) -> QuerySet[_T]: ...
    def intersection(self, *other_qs: Any) -> QuerySet[_T]: ...
    def difference(self, *other_qs: Any) -> QuerySet[_T]: ...
    def select_for_update(self, nowait: bool = ..., skip_locked: bool = ..., of: Tuple = ...) -> QuerySet: ...
    def select_related(self, *fields: Any) -> QuerySet[_T]: ...
    def prefetch_related(self, *lookups: Any) -> QuerySet[_T]: ...
    def annotate(self, *args: Any, **kwargs: Any) -> QuerySet[_T]: ...
    def order_by(self, *field_names: Any) -> QuerySet[_T]: ...
    def distinct(self, *field_names: Any) -> QuerySet[_T]: ...
    def extra(
        self,
        select: Optional[Dict[str, Any]] = ...,
        where: Optional[List[str]] = ...,
        params: Optional[List[Any]] = ...,
        tables: Optional[List[str]] = ...,
        order_by: Optional[Sequence[str]] = ...,
        select_params: Optional[Sequence[Any]] = ...,
    ) -> QuerySet[_T]: ...
    def reverse(self) -> QuerySet[_T]: ...
    def defer(self, *fields: Any) -> QuerySet[_T]: ...
    def only(self, *fields: Any) -> QuerySet[_T]: ...
    def using(self, alias: Optional[str]) -> QuerySet[_T]: ...

one of

    def earliest(self, *fields: Any, field_name: Optional[Any] = ...) -> _T: ...
    def latest(self, *fields: Any, field_name: Optional[Any] = ...) -> _T: ...
    def first(self) -> Optional[_T]: ...
    def last(self) -> Optional[_T]: ...

and so on.

And thank you very much for your efforts!

@mkurnikov
Copy link
Member

Unfortunately, right now I believe you would have to e.g. write QuerySet[MyModel, MyModel] to avoid having mypy type it as QuerySet[MyModel, Any].

We can hide it from users by leveragive get_type_analyze_hook:

def set_first_generic_param_as_default_for_second(ctx: AnalyzeTypeContext) -> Type:
    if not ctx.type.args:
        return ctx.api.named_type(helpers.QUERYSET_CLASS_FULLNAME, [AnyType(TypeOfAny.explicit),
                                                                    AnyType(TypeOfAny.explicit)])
    args = ctx.type.args
    if len(args) == 1:
        args = [args[0], args[0]]

    analyzed_args = [ctx.api.analyze_type(arg) for arg in args]
    # TODO: this function should be made into an object with __call__, and initialized with fullname,
    # s/helpers.QUERYSET_CLASS_FULLNAME/fullname
    return ctx.api.named_type(helpers.QUERYSET_CLASS_FULLNAME, analyzed_args)

def get_type_analyze_hook(self, fullname: str
                              ) -> Optional[Callable[[AnalyzeTypeContext], Type]]:
        # TODO: instead of just queryset, it should be in the same style as _get_current_model_bases/_get_current_manager_bases/...
        if fullname == 'django.db.models.query.QuerySet':
            return set_first_generic_param_as_default_for_second

Please, address TODOs, see also python/mypy#6503.

Error messages should also be changed, but let's do it some time later, as it should also be done for all Field subclasses, see python/mypy#6501

@syastrov
Copy link
Contributor Author

syastrov commented Mar 5, 2019

That sounds like a good plan by using get_type_analyze_hook. I'll see if I get the time to implement this soon.

- Use correct return type for QuerySet.dates() / QuerySet.datetimes().
- Use correct type params in return type for QuerySet.__and__ / QuerySet.__or__
- Re-add Sized as base class for QuerySet.
- Add test of .all() for all _Row types.
- Add test of .get() for all _Row types.
- Remove some redundant QuerySet method tests.
@syastrov syastrov force-pushed the queryset_values_and_values_list branch from 342bcae to 5f71ccb Compare March 7, 2019 07:23
@syastrov
Copy link
Contributor Author

syastrov commented Mar 7, 2019

I think it's ready for your review again now :)

I made a few more changes to fix some small issues I noticed as well:

  • Fix return type for QuerySet.select_for_update() (missing type params)
  • I have added correct return type for QuerySet.dates() / QuerySet.datetimes()
  • Added correct type params in return type for QuerySet.__and__ / QuerySet.__or__

@mkurnikov
Copy link
Member

PR looks great, resolve conflicts with master again and I'll merge.

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.

2 participants