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 #35326 -- added allow_overwrite
parameter to FileSystemStorage.
#18020
base: main
Are you sure you want to change the base?
Conversation
e9653cd
to
027188b
Compare
027188b
to
0ec0a4d
Compare
What about using the |
OK, so then I would add an |
ce1431e
to
59124ab
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 for this @bcail! I have initial comments 👍
59124ab
to
46802d8
Compare
@sarahboyce Thanks for your review. I've added I added a commit with a warning if the default |
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.
I added a commit with a warning if the default OS_OPEN_FLAGS is overridden.
Thank you for doing this @bcail, I think this approach looks good to me.
I have a few minor comments, this is getting really close 👍
bb1bfef
to
a876585
Compare
Kind of derailed the original ticket so would rename the title |
allow_overwrite
parameter to FileSystemStorage.
Thank you for the updates @bcail, have you seen this suggestion? |
@sarahboyce I wonder if there would be any downstream code that would want to call But I'll do it whichever way you think is best. |
To clarify my thoughts, if we look at the docs for
I believe it is available for a new file when we are in overwrite mode regardless of whether a file already exists (though we might want to slightly tweak the wording of the true case). I also believe this would better suit the documented approach for writing a custom file storage system (i.e. defining
Thank you for raising this. 👍 @jschneier do you have an opinion here? |
f2b4ec1
to
6375678
Compare
Thank you @bcail 🥳 this looks great, I pushed up small edits and will seek a second opinion before merging 👍 |
Thanks, @sarahboyce. |
6375678
to
c168952
Compare
Trac ticket number
ticket-35326
Branch description
This patch handled temporary uploaded files when the filesystem storage allows overwriting files.
Checklist
main
branch.