-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Phase 1 Added new method download_blob_to_file within Storage Client #7949
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This doesn't quite line up with the line above it (there's one extra space here)
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.
Fixed!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give an example of the URI gs://bucket_name/path/to/blob
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. Should be two slashes, one colon. gs://
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.
Fixed typo!
All nox tests pass.
|
Part of issue: #7762
@tswast
@engelke