-
-
Notifications
You must be signed in to change notification settings - Fork 867
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
Write mode option for Dropbox #873
Conversation
storages/backends/dropbox.py
Outdated
@@ -89,6 +93,9 @@ def delete(self, name): | |||
self.client.files_delete(self._full_path(name)) | |||
|
|||
def exists(self, name): | |||
if self.overwrite: |
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.
This is wrong.
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.
so how then to make Django to not rename? any suggestions?
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.
Look at the other backends. They override get_available_name
.
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.
@jschneier updated
storages/backends/dropbox.py
Outdated
@@ -132,7 +139,7 @@ def _open(self, name, mode='rb'): | |||
def _save(self, name, content): | |||
content.open() | |||
if content.size <= self.CHUNK_SIZE: | |||
self.client.files_upload(content.read(), self._full_path(name)) | |||
self.client.files_upload(content.read(), self._full_path(name), mode=WriteMode('overwrite')) |
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.
The mode isn't constant. It needs to depend on what the user wants.
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.
django checks for exists, if it's true - it renames, so the mode does not actually metter
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.
@jschneier so, can I then add WriteMode as an option?
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.
@jschneier updated, made as an option
storages/backends/dropbox.py
Outdated
@@ -158,3 +166,24 @@ def _chunked_upload(self, content, dest_path): | |||
content.read(self.CHUNK_SIZE), cursor | |||
) | |||
cursor.offset = content.tell() | |||
|
|||
def _clean_name(self, name): |
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 think you can just use self._full_path
instead here since that seems to work.
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.
done
Overwrite an existing file when it has the same name as the file being uploaded. | ||
Otherwise, rename it. Default is ``False`` | ||
|
||
``DROPBOX_WRITE_MODE`` (optional) |
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.
Can you default this to whatever the current default is (not overwrite). Don't want to break backwards compat. Can you also link out to the Dropbox docs for this property.
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.
changed to 'add'
storages/backends/dropbox.py
Outdated
def get_available_name(self, name, max_length=None): | ||
"""Overwrite existing file with the same name.""" | ||
name = self._clean_name(name) | ||
if self.file_overwrite: |
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.
Here you can just check if self.write_mode == 'overwrite'
.
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.
updated
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.
@jschneier can you review please ?
Looks great. @petriichuk did you test it? |
of course, I already use it. |
* added overwrite option * added myself as AUTHOR * Refactored to native feature implementation after comment from package owner * Make write mode as external option, with default set to overwrite * Fix order of imports * added new options description * fixed import sort issue * removed redundand option, updated documentation * replaced _clean_name by _full_path * update author change comment * removed unused import posixpath Co-authored-by: Taras Petriichuk <taraspetriichuk@TX-N-0432.local>
* added overwrite option * added myself as AUTHOR * Refactored to native feature implementation after comment from package owner * Make write mode as external option, with default set to overwrite * Fix order of imports * added new options description * fixed import sort issue * removed redundand option, updated documentation * replaced _clean_name by _full_path * update author change comment * removed unused import posixpath Co-authored-by: Taras Petriichuk <taraspetriichuk@TX-N-0432.local>
Adding the possibility to overwrite existing files at Dropbox instead of allowing Django change it's name.