Phase 1 Added new method download_blob_to_file within Storage Client#7949
Phase 1 Added new method download_blob_to_file within Storage Client#7949tswast merged 4 commits intogoogleapis:masterfrom
Conversation
tswast
left a comment
There was a problem hiding this comment.
Looking good! A few comments on how to improve the docstrings. Your approach looks good, though I'm curious why you didn't follow the same pattern as you did in the create_bucket function.
| Args: | ||
| blob_or_uri (Union[ \ | ||
| :class:`~google.cloud.storage.blob.Blob`, \ | ||
| str, \ |
There was a problem hiding this comment.
Nit: This doesn't quite line up with the line above it (there's one extra space here)
| blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end) | ||
| except AttributeError: | ||
| scheme, netloc, path, query, frag = urlsplit(blob_or_uri) | ||
| bucket = Bucket(self, name=netloc) |
There was a problem hiding this comment.
Let's raise a ValueError if scheme isn't gs. I was warned that people might start passing in http URLs and expect this to work.
There was a problem hiding this comment.
We'll need a test for this check. (Try a https:// link and verify that ValueError is raised with pytest.raises
There was a problem hiding this comment.
I added an if statement for raising ValueError if scheme isn't gs as directed. I also added the test for the change.
| :class:`~google.cloud.storage.blob.Blob`, \ | ||
| str, \ | ||
| ]): | ||
| The blob resource to pass or URI to download. |
There was a problem hiding this comment.
Let's give an example of the URI gs://bucket_name/path/to/blob
There was a problem hiding this comment.
Added examples for URI and if passing in a resource object.
| """ | ||
| try: | ||
| blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end) | ||
| except AttributeError: |
There was a problem hiding this comment.
What's the reason for looking for AttributeError rather than checking isinstance like you did in create_bucket?
I had initially followed the pattern and was using IsInstance, but ran into issues when I was attempting to test the function. Andrew suggested I scrap my code and go with using try/except which would be more Pythonic and easier to test. |
|
|
||
| >>> with open('file-to-download-to') as file_obj: | ||
| >>> client.download_blob_to_file( | ||
| >>> 'gs::/bucket_name/path/to/blob', file) |
There was a problem hiding this comment.
Typo. Should be two slashes, one colon. gs://
|
All nox tests pass. |
Part of issue: #7762
@tswast
@engelke