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

refactor: move methods to client class #311

Merged
merged 7 commits into from
Dec 11, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #38

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 12, 2020
@HemangChothani HemangChothani changed the title Storage issue 38 refactor: move methods to clisnt class Nov 12, 2020
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Nov 12, 2020
@HemangChothani HemangChothani changed the title refactor: move methods to clisnt class refactor: move methods to client class Nov 12, 2020
Copy link
Contributor

@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewsg
Copy link
Contributor

andrewsg commented Dec 1, 2020

@tswast In the bug you said, "we need to update samples for moved methods before deprecating the old ones." Is this change okay as-is, or do we need to make this change without the deprecation warning first, then update samples, then add the deprecation warning?

@tswast
Copy link
Contributor

tswast commented Dec 1, 2020

If it helps you find the samples that need to be updated, I'm okay merging this PR first. It's just a bad look if we have samples on cloud.google.com that show deprecation warnings.

@andrewsg
Copy link
Contributor

andrewsg commented Dec 1, 2020

I agree. But we can't update the sample without pushing part of this change, so @HemangChothani Let's omit the deprecation warning for now and open an issue to update the samples, then add the deprecation warning in that order.

@HemangChothani
Copy link
Contributor Author

@andrewsg Opend an issue GoogleCloudPlatform/python-docs-samples#5031 to update the samples.

@andrewsg andrewsg added the automerge Merge the pull request once unit tests and other checks pass. label Dec 11, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 5b4568e into googleapis:master Dec 11, 2020
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 11, 2020
@HemangChothani
Copy link
Contributor Author

@andrewsg I have opend an issue to update sample GoogleCloudPlatform/python-docs-samples#5031 in python-docs-sample repo , but i didn't find any sample needs to update, should i open issue in python-storage repo to update snippets and system-tests for list_blobs and download_to_file methods

@frankyn
Copy link
Member

frankyn commented Dec 16, 2020

@andrewsg and @HemangChothani, I replied to the issue. We are only missing updates for download_to_filename.

@HemangChothani
Copy link
Contributor Author

@frankyn we have moved download_to_file into the client class, download_to_filename is still in the blob class so i think there is no need to update any sample.

blob.download_to_filename is calling blob.download_to_file method which is deprecated now, so i will update soon and direct call the method client.download_to_file method.

Also update the method call from bucket.list_blobs to client.list_blobs.

@frankyn
Copy link
Member

frankyn commented Dec 17, 2020

Why was only download_to_file updated but not download_to_filename?

@HemangChothani
Copy link
Contributor Author

Document mentioned in issue #38, found download_to_file and list_blobs methods need to move implementation from blob class to client class and deprecate older one.

cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: move API-methods to client
4 participants