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

[GCP-5538] Storage: Missing support for HTTP Headers and Query String Parameters #17

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HemangChothani
Copy link
Collaborator

@HemangChothani HemangChothani commented Jul 31, 2019

Addressing #5538

@HemangChothani HemangChothani changed the title Storage: Fix/add support for http headers and query parameters/5538 Storage: Add support for http headers and query parameters/5538 Jul 31, 2019
@mf2199 mf2199 changed the title Storage: Add support for http headers and query parameters/5538 [GCP-5538] Storage: Missing support for HTTP Headers and Query String Parameters Jul 31, 2019
@mf2199 mf2199 requested review from sumit-ql, emar-kar and IlyaFaer July 31, 2019 17:36
storage/google/cloud/storage/client.py Show resolved Hide resolved
blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end)
blob_or_uri.download_to_file(
file_obj, client=self, start=start, end=end, query_params=query_params
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This repeats lines 402-404. Is there any way to check the type and validate the blob_or_uri parameter before the try...except clause and avoid an unnecessary API call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have only added query_params parameter not added try..except and is it fine to change their usecase?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be fine as long as two API calls were there initially, I just thought we could improve on that too, along the way.

storage/google/cloud/storage/client.py Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/google/cloud/storage/blob.py Outdated Show resolved Hide resolved
storage/tests/unit/test_blob.py Outdated Show resolved Hide resolved
client=client,
start=start,
end=end,
query_params=query_params,
Copy link

Choose a reason for hiding this comment

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

Remove the extra "comma" after query params.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if i remove "comma" than blacken test failed, you can see also in client.py that added "comma" after client parameter

client=client,
start=start,
end=end,
query_params=query_params,
Copy link

Choose a reason for hiding this comment

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

Remove extra comma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

size,
num_retries,
predefined_acl,
extra_headers,
Copy link

Choose a reason for hiding this comment

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

Remove "comma" after extra_headers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as above

Copy link

@emar-kar emar-kar left a comment

Choose a reason for hiding this comment

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

LGTM
Nevertheless I'm really advising to follow @mf2199 advises about custom spelling.

@HemangChothani
Copy link
Collaborator Author

Yes , i did that, but i followed docs url where i found Extension (Custom) HTTP Headers so i used same as documented.

blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end)
blob_or_uri.download_to_file(
file_obj, client=self, start=start, end=end, query_params=query_params
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be fine as long as two API calls were there initially, I just thought we could improve on that too, along the way.

storage/google/cloud/storage/client.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants