-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
Fixed #36504 -- Clarified docs for using with FileField storage. #19725
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
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.
Thanks for taking this up!
I think the aim of the ticket is to remove the suggestion to use LazyObject
altogether. (I don't know if there's a possible suggestion we can replace it with.)
cc'ing the folks (@AhmedNassar7 and @sarahboyce) who added the suggestion in ticket-36269 in case we might be missing something
If we are to remove it, we would need to reopen ticket-36504 Essentially what the example was hoping to do was to mimic how |
Did a bit of digging and I figured out why it doesn't work: The signal that resets storages’ cached value when settings change only does it explicitly for Lines 114 to 132 in cd0966c
So maybe we may close this ticket by adding a custom signal receiver to the section added in #19349? WDYT something like: @receiver(setting_changed)
def reset_other_storage(*, setting, **kwargs):
if setting in ("STORAGES",):
other_storage._wrapped = empty |
That's a great idea 👏 |
Friendly reminder @khosro-web, when you have updated the PR per the discussions in the review, and are ready for another review, please update the ticket flags in Trac, per these docs, specifically, you need to uncheck "Patch needs improvement" |
Trac ticket number
Fixed #36504
Branch description
This PR clarifies the Django documentation regarding the usage of
override_settings
withFileField
storage.It explains why using a simple callable for the
storage
argument may not behave as expected in tests due to early evaluation at import time.It adds a note recommending the use of a
LazyObject
subclass pattern to delay storage evaluation, ensuringoverride_settings
works properly in test scenarios.Checklist
main
branch.