Skip to content

Isolating Connection objects in _http modules (where relevant). #3057

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 24, 2017
Merged

Isolating Connection objects in _http modules (where relevant). #3057

merged 4 commits into from
Feb 24, 2017

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 23, 2017 •
edited
Loading

This is a work-in-progress for now, but vision and speech are done. Will also be doing logging, pubsub and maybe error_reporting.

FWIW, this is a result of me working on #2746.

@dhermes dhermes added api: clouderrorreporting Issues related to the Error Reporting API. api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. api: speech Issues related to the Speech-to-Text API. api: vision Issues related to the Cloud Vision API. hygiene labels Feb 23, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 23, 2017
daspecster
daspecster previously approved these changes Feb 23, 2017
View reviewed changes
Copy link
Contributor

@daspecster daspecster left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM pending Travis/Circle.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2017

@daspecster Thanks but you are bit quick on the draw. The work-in-progress bit means more changes will be coming.

@dhermes dhermes force-pushed the move-connection-off-client branch from a6375d8 to ee24838 Compare February 23, 2017 19:27
@daspecster
Copy link
Contributor

daspecster commented Feb 23, 2017

Sorry, I thought that meant this was one PR of n-PRs.

@lukesneeringer lukesneeringer added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 23, 2017
@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2017

Holding off on logging and pubsub in this PR since it would un-necessarily create 3 connections (since there are 3 services created in either case). What we really care about is connection.api_request (here and here), which just relies on CLASS attributes, the credentials (known on the client) and the inputs. So it can wait until a larger refactor.

@dhermes dhermes force-pushed the move-connection-off-client branch from 3dfea5c to 7e29a3d Compare February 23, 2017 20:47
@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2017

@daspecster @lukesneeringer PTAL this is ready for review

@dhermes dhermes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 23, 2017
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Feb 23, 2017
@dhermes
Removing mock.Mock() in error reporting.
64c1b3c 
This actually required a much more comprehensive set of
unit test changes than expected.

Also incorporated a change from googleapis#3057 (which slipped through
due to overly broad mocks).
@dhermes dhermes mentioned this pull request Feb 23, 2017
Merged
@daspecster
Copy link
Contributor

daspecster commented Feb 23, 2017

This LGTM.

@dhermes dhermes merged commit f66ac9d into googleapis:master Feb 24, 2017
@dhermes dhermes deleted the move-connection-off-client branch February 24, 2017 17:26
@dhermes dhermes mentioned this pull request Feb 24, 2017
Merged
19 tasks
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Mar 1, 2017
@dhermes
Removing mock.Mock() in error reporting.
80ec376 
This actually required a much more comprehensive set of
unit test changes than expected.

Also incorporated a change from googleapis#3057 (which slipped through
due to overly broad mocks).
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#3057 from dhermes/move-connection-off-c…
ccd9886 
…lient

Isolating Connection objects in _http modules (where relevant).
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Removing mock.Mock() in error reporting.
cf60802 
This actually required a much more comprehensive set of
unit test changes than expected.

Also incorporated a change from googleapis#3057 (which slipped through
due to overly broad mocks).
parthea pushed a commit that referenced this pull request Oct 21, 2023
@nnegrey @busunkim96
automl: fix batch test due to error message change [(#3057)](GoogleCl…
9269949 
…oudPlatform/python-docs-samples#3057)

fix: GoogleCloudPlatform/python-docs-samples#3052
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@lukesneeringer lukesneeringer lukesneeringer approved these changes

@daspecster daspecster daspecster left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

Labels

api: clouderrorreporting Issues related to the Error Reporting API. api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. api: speech Issues related to the Speech-to-Text API. api: vision Issues related to the Cloud Vision API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dhermes @daspecster @lukesneeringer @tseaver @googlebot