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

Refs #34007 -- Added Q.referenced_based_fields property #18093

Merged

Conversation

shangxiao
Copy link
Contributor

@shangxiao shangxiao commented Apr 21, 2024

Refs ticket-34007 & ticket-35359
Cherry picked from #16054 to be used in #18068

Notes:

  • I changed @property into @cached_property, this wasn't reviewed so will need to be confirmed whether this is acceptable
  • Test was updated with felix's suggestion to mix combining all params & subtest

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @shangxiao for this work. Looks good!

I saw in the PR description that you are wondering about how correct is to switch to cached_property. I am too! Was there a specific reason to choose this switch? Tagging @charettes to seek help answering this.

django/db/models/query_utils.py Outdated Show resolved Hide resolved
django/db/models/query_utils.py Outdated Show resolved Hide resolved
@charettes
Copy link
Member

@nessita I think that using a cached_property should be fine as Q objects, like most expressions-like objects, are not meant to be altered in place but cloned before hand. This is not different from the other cached_property we have on Expression properties.

@sarahboyce sarahboyce force-pushed the refs_34007_referenced_base_fields branch from 08742a8 to d648bd7 Compare May 2, 2024 10:22
@sarahboyce sarahboyce requested review from felixxm and nessita May 2, 2024 10:22
@sarahboyce sarahboyce force-pushed the refs_34007_referenced_base_fields branch from d648bd7 to fb69337 Compare May 2, 2024 10:27
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks great with the suggested replacement.

django/db/models/query_utils.py Outdated Show resolved Hide resolved
@sarahboyce sarahboyce force-pushed the refs_34007_referenced_base_fields branch from fb69337 to bd7d4d3 Compare May 2, 2024 11:56
Thank you to Mariusz Felisiak and Natalia Bidart for the reviews.
@sarahboyce sarahboyce force-pushed the refs_34007_referenced_base_fields branch from bd7d4d3 to e9080cc Compare May 2, 2024 16:41
@sarahboyce sarahboyce merged commit 97d48cd into django:main May 2, 2024
17 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants