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

Fixed #30446 -- Resolved Value.output_field for stdlib types. #11359

Merged
merged 2 commits into from
Jul 15, 2020

Conversation

charettes
Copy link
Member

No description provided.

@charettes
Copy link
Member Author

This still needs a bit of work when combining fields from different type (e.g. (Avg('float_field') + 2) * 3) must resolve to FloatField but right now it errors out with FieldError('Expression contains mixed types. You must set output_field.').

This last problem has a bit of overlap with #6395.

@charettes charettes force-pushed the value-resolve-output-field branch 3 times, most recently from 4404c54 to 914209c Compare October 8, 2019 14:00
@charettes charettes force-pushed the value-resolve-output-field branch 4 times, most recently from 26e4871 to 53c40a7 Compare June 5, 2020 04:08
Copy link
Sponsor Member

@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.

This is really cool and tidies up a bunch of use cases!

tests/expressions/tests.py Outdated Show resolved Hide resolved
@charettes
Copy link
Member Author

charettes commented Jun 6, 2020

The last commit in the series serves more as an example of what kind of simplification this allows. It could be reverted if we want to preserve the original nature of tests, just thought it could be worth removing a bit of boilerplate.

This PR makes most of the remaining Value usages unnecessary (except for str wrapping since the ORM defaults to column references) just like explicit passing of Value(output_field). I believe it might be worth a mention in the release notes that I'm happy to write given the nice improvements of life it brings but I'll defer to the reviewers before committing to it.

I guess we could expose an API for contrib apps to register type -> output_field mapping (e.g. List[T] -> contrib.postgres.ArrayField[T]) but this can be done in a following PR/ticket.

Finally, kind of rant but I really wish we named this abstraction Literal instead of Value which doesn't really convey it's purpose and cause language gymnastic due to its vagueness (e.g. is_direct_value instead of is_literal). Not convinced we should rename it given the limited benefit it would bring at this point though.

@adamchainz
Copy link
Sponsor Member

I believe it might be worth a mention in the release notes that I'm happy to write given the nice improvements of life it brings but I'll defer to the reviewers before committing to it.

This is definitely worth mentioning with a short example. I think most Django projects end up using Value() somewhere.

I really wish we named this abstraction Literal instead of Value which doesn't really convey it's purpose

Yes Literal is a marginally better name but probably not worth the change. It would also conflict with typing.Literal if anyone uses that in the same file.

@felixxm
Copy link
Member

felixxm commented Jun 16, 2020

@charettes Thanks 👍 I will try to first handle all cleanups. I moved the first one to the #13070.

@felixxm
Copy link
Member

felixxm commented Jun 24, 2020

Merged two commits in ea3beb4, 5227101.

@charettes
Copy link
Member Author

Thanks for the merges @felixxm I should have time to address the remaining points regarding documentation and for_save this week-end.

@charettes charettes force-pushed the value-resolve-output-field branch 3 times, most recently from e844dc0 to 798102c Compare July 3, 2020 13:17
@charettes
Copy link
Member Author

Alright pushed a commit that restored the for_save = False and details its reasoning and docs and release notes.

@charettes
Copy link
Member Author

buildbot, test on oracle.

@felixxm
Copy link
Member

felixxm commented Jul 14, 2020

I moved GIS cleanup to the #13185 and merged two cleanups in f783a99 and 9c5c9bd.

@felixxm felixxm force-pushed the value-resolve-output-field branch 3 times, most recently from ee35218 to cb0a181 Compare July 14, 2020 07:42
@felixxm felixxm force-pushed the value-resolve-output-field branch from bbee836 to 1ba5e47 Compare July 15, 2020 08:33
@felixxm
Copy link
Member

felixxm commented Jul 15, 2020

@charettes Thanks 👍 I pushed small edits and removed output_field in some tests.

@felixxm felixxm force-pushed the value-resolve-output-field branch from 1ba5e47 to 945a05f Compare July 15, 2020 08:54
charettes and others added 2 commits July 15, 2020 10:58
This required implementing a limited form of dynamic dispatch to combine
expressions with numerical output. Refs #26355 should eventually provide
a better interface for that.
@felixxm felixxm force-pushed the value-resolve-output-field branch from 945a05f to 156a213 Compare July 15, 2020 08:58
@felixxm felixxm merged commit 156a213 into django:master Jul 15, 2020
@charettes charettes deleted the value-resolve-output-field branch July 15, 2020 13:59
@charettes
Copy link
Member Author

Thanks for the adjustments and merge Mariusz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants