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 #35405 -- Used @cached_property in FieldCacheMixin. #18101
base: main
Are you sure you want to change the base?
Conversation
a665863
to
424b695
Compare
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.
Thank you @adamchainz for this work! I agree with Simon that the patch looks good but I think this also needs a few tests covering the deprecation paths/messages and how both the old method and new cached property work OK.
973f80f
to
86d51aa
Compare
I have added some tests specific to the mixin, including for the deprecation path. |
co-authored-by: Simon Charette <charette.s@gmail.com> co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
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.
Branch looks great! I think is ready to checkin pending the adding of deprecation comments to the new test file so we can later cleanup accordingly. Thank you @adamchainz!
from .models import Foo | ||
|
||
|
||
class ExampleOld(FieldCacheMixin): |
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.
These are great tests, thank you for adding them. But, I think we should add some RemovedInDjango60Warning
comments through this file to ensure we do the proper cleanup also at the test level when we bootstrap Django 6.0. Would you agree?
Trac ticket number
ticket-35405
Branch description
Optimization as described on ticket.
Checklist
main
branch.