[gcloud] Add support for MIME type #320
Conversation
Codecov Report
@@ Coverage Diff @@
## master #320 +/- ##
==========================================
+ Coverage 75.58% 75.61% +0.03%
==========================================
Files 11 11
Lines 1544 1546 +2
==========================================
+ Hits 1167 1169 +2
Misses 377 377
Continue to review full report at Codecov.
|
|
I also checked that this is working on my site. |
|
Why not use the |
|
@svengt I did not know about this function. Thanks for it! I fixed it now. |
|
@jschneier or @scjody can you please Merge it or tell me what should be fixed? Thanks |
|
Looks fine with me but I don't know gcloud well enough to decide on it. It needs rebasing, by the way. Please squash your commits together and rebase on master. |
| @@ -20,6 +21,7 @@ | |||
| class GoogleCloudFile(File): | |||
| def __init__(self, name, mode, storage): | |||
| self.name = name | |||
| self.mime_type = mimetypes.guess_type(name)[0] | |||
There was a problem hiding this comment.
Since guess_type isn't 100% reliable, I think we need to allow the user to override the type here if needed. Can you add that?
There was a problem hiding this comment.
When the user should override it? Globally at the Django settings (set the desired MIME type for specific file extension)? Something like:
GS_MIME_TYPE = {
'.css': 'text/css',
'.mp4': 'video/mp4',
}
I didn't add it because MIME types are not THAT important. They can be edited manually in Google Cloud Console if really needed and almost everything should work also without them. So I didn't want to add complexity with another settings option.
But if you thinks it is needed I can definitely add this to the code.
There was a problem hiding this comment.
Now that I've looked more closely, I don't see a good place to add this. Django's storage API doesn't have any support for MIME types as far as I can tell, so we're well within our rights to set whatever we want when saving a file to the storage provider.
storages/backends/gcloud.py
Outdated
| @@ -153,7 +155,7 @@ def _save(self, name, content): | |||
| content.name = cleaned_name | |||
| encoded_name = self._encode_name(name) | |||
| file = GoogleCloudFile(encoded_name, 'rw', self) | |||
| file.blob.upload_from_file(content, size=content.size) | |||
| file.blob.upload_from_file(content, size=content.size, content_type=file.mime_type) | |||
There was a problem hiding this comment.
It's better to wrap lines at 79 columns (although the Django Coding Style allows up to 119 if needed for readability). Here you can put content_type=... on the next line without hurting readability, so please do.
tests/test_gcloud.py
Outdated
| @@ -91,7 +92,7 @@ def test_open_write(self, MockBlob): | |||
| # File data is not actually written until close(), so do that. | |||
| f.close() | |||
|
|
|||
| MockBlob().upload_from_file.assert_called_with(tmpfile) | |||
| MockBlob().upload_from_file.assert_called_with(tmpfile, content_type=mimetypes.guess_type(self.filename)[0]) | |||
There was a problem hiding this comment.
Here it would be preferable to put content_type on the next line. This will still make the line longer than 79 characters but that's OK.
There was a problem hiding this comment.
Thanks for the style changes and the discussion about overrides. I'm OK with merging this. @jschneier is this OK with you?
| @@ -20,6 +21,7 @@ | |||
| class GoogleCloudFile(File): | |||
| def __init__(self, name, mode, storage): | |||
| self.name = name | |||
| self.mime_type = mimetypes.guess_type(name)[0] | |||
There was a problem hiding this comment.
Now that I've looked more closely, I don't see a good place to add this. Django's storage API doesn't have any support for MIME types as far as I can tell, so we're well within our rights to set whatever we want when saving a file to the storage provider.
|
I'm okay with this change in principle across the storages. Since this is something that has been being fixed piecemeal in all of the cloud backends I wonder if there is a nicer general solution.
… On Jun 28, 2017, at 15:02, Jody McIntyre ***@***.***> wrote:
@scjody approved this pull request.
Thanks for the style changes and the discussion about overrides. I'm OK with merging this. @jschneier is this OK with you?
In storages/backends/gcloud.py:
> @@ -20,6 +21,7 @@
class GoogleCloudFile(File):
def __init__(self, name, mode, storage):
self.name = name
+ self.mime_type = mimetypes.guess_type(name)[0]
Now that I've looked more closely, I don't see a good place to add this. Django's storage API doesn't have any support for MIME types as far as I can tell, so we're well within our rights to set whatever we want when saving a file to the storage provider.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
OK squashed and rebased. You can merge it. |
Resolves #303 Issue