Skip to content
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

Merged
merged 4 commits into from
May 15, 2019

Conversation

lbristol88
Copy link
Contributor

Part of issue: #7762

@tswast
@engelke

@lbristol88 lbristol88 requested a review from crwilcox as a code owner May 10, 2019 20:45
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 10, 2019
@tswast tswast self-requested a review May 10, 2019 21:51
@tswast tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 10, 2019
Copy link
Contributor

@tswast tswast left a 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, \
Copy link
Contributor

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)

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.
Copy link
Contributor

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

Copy link
Contributor Author

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:
Copy link
Contributor

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?

@lbristol88
Copy link
Contributor Author

lbristol88 commented May 10, 2019

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.

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)
Copy link
Contributor

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://

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed typo!

@lbristol88
Copy link
Contributor Author

All nox tests pass.

All done! ✨ 🍰 ✨
27 files left unchanged.
nox > Session blacken was successful.
nox > Ran multiple sessions:
nox > * default: success
nox > * unit-2.7: success
nox > * unit-3.5: success
nox > * unit-3.6: success
nox > * unit-3.7: success
nox > * system-2.7: success
nox > * system-3.6: success
nox > * cover: success
nox > * lint: success
nox > * lint_setup_py: success
nox > * blacken: success

@tswast tswast added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2019
@tswast tswast merged commit e7a7bd4 into googleapis:master May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants