-
-
Notifications
You must be signed in to change notification settings - Fork 866
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
Improving azure storage #45
Conversation
Using MEDIA_URL from settings as newer versions of django default storage does.
@@ -57,9 +57,20 @@ def size(self, name): | |||
return properties["content-length"] | |||
|
|||
def _save(self, name, content): | |||
if hasattr(content.file, 'content_type'): |
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 assumes we have a ContentFile
right? I know that's what we return from _open
but is this going to be the case generally?
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.
As i have interpreted the Django docs[1] a custom storage should handle File objects [2]. Thus it should always have a file attribute. This code checks whether the file has a content_type, if not it tries to guess it.
Please correct me if I have not interpreted the docs correctly ;)
[1] https://docs.djangoproject.com/en/1.8/howto/custom-file-storage/
[2] https://docs.djangoproject.com/en/1.8/ref/files/file/
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.
You are correct, sorry about that.
Thanks for your contribution! I've left one concern of mine in line but looks nice other than that. |
if hasattr(content.file, 'content_type'): | ||
content_type = content.file.content_type | ||
else: | ||
content_type = mimetypes.guess_type(name)[0] |
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 forgot to import mimetypes
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.
Nice spot. I'll fix :)
Can someone merge this pull request? |
Merge please. |
+1 |
Anyone with write access who can merge this? |
Sorry for the delay folks, I have merged this. I realize that Azure storage probably wasn't really usable without that |
No description provided.