-
-
Couldn't load subscription status.
- Fork 442
Support annotations and joins in F() #1761
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
Conversation
1d68e34 to
5f45331
Compare
Pull Request Test Coverage Report for Build 11742066623Details
💛 - Coveralls |
5f45331 to
aadc8f9
Compare
| res = ResolveResult( | ||
| term=term, | ||
| joins=function_arg.joins, | ||
| output_field=function_arg.output_field, # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you had to use so many type ignores?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! This is something I never ran into before. Basically the Field class has the following type checking code:
# These methods are just to make IDE/Linters happy:
if TYPE_CHECKING:
def __new__(cls, *args: Any, **kwargs: Any) -> "Field[VALUE]":
return super().__new__(cls)
@overload
def __get__(self, instance: None, owner: Type["Model"]) -> "Field[VALUE]": ...
@overload
def __get__(self, instance: "Model", owner: Type["Model"]) -> VALUE: ...
def __get__(
self, instance: Optional["Model"], owner: Type["Model"]
) -> "Field[VALUE] | VALUE": ...
def __set__(self, instance: "Model", value: VALUE) -> None: ...
that makes it possible to have type hints for both the Model classes but also for Model instances. However, this seems to limit who can have the field attribute to only Model and its subclasses because of instance and owner arg typing.
I added the Field property to ResolveResult to be able to use Field for type conversions and this is what caused warnings that are being ignored.
I tried to add overloaded versions of __get__ to Field but this looks ugly because Field has to know about the ResolveResult:
@overload
def __get__(self, instance: None, owner: Type["Model"]) -> "Field[VALUE]": ...
@overload
def __get__(self, instance: "Model", owner: Type["Model"]) -> VALUE: ...
@overload
def __get__(
self, instance: ResolveResult, owner: Type["ResolveResult"]
) -> "Field[VALUE]": ...
def __get__(
self,
instance: Union[Optional["Model"], ResolveResult],
owner: Union[Type["Model"], Type[ResolveResult]],
) -> "Field[VALUE] | VALUE": ...
Please let me know if you have ideas on how to handle this better, thanks!
Description
This PR:
F(), e.g.F("author__name")that automatically introduces joins to the queryF(), e.g.IntFields.annotate(intnum_plus_1=F("intnum") + 1).annotate(intnum_plus_2=F("intnum_plus_1") + 1)In order to achieve that, the following implementation details changed
Expression.resolvewas unified to always return aResolveResult. Previously it could have been either a tuple or dictionary.Fwas changed to be anExpressioninstead of pypika'sField. This allowed to move theF's resolution code insideF.resolveand simplify the resolution code in general becauseFcan be treated as otherExpressions.CombinedExpressionwas introduced to handle arithmetic operations onF(), e.g.F("a") + 1. It was inspired by Django's CombinedExpression and replaces pypika's arithmetic.CombinedExpressionis a child ofExpression, so it can be resolved without knowing the exact type of the object as opposed to pypika'sArithmeticExpressionthat was used previosly.As part of this PR, I had to change how custom functions can be defined. It changed from
to
Effectively now custom functions are supposed to be a tortoise
Function, not a Pypika'sFunction. The change had to be done because the Pypika'sFunctionis no longer compatible withFand, frankly, having the interface that accepts both Pypika's functions and tortoise functions is a bit messy and makes the code quite convoluted.Motivation and Context
This introduces functionality supported by Django but also there is a clear demand for these features in the tortoise community. The following issues should be fixed by this PR
Model.annotate(...).update(...)not working #1703How Has This Been Tested?
Checklist: