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

Small tweaks to Connection in storage; towards a cleaner API. #587

Closed
wants to merge 1 commit into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jan 31, 2015

  • Making Connection.make_request non-public
  • Making Connection.build_api_url a class method

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 53f6b86 on dhermes:storage-connection-tweaks into d858ff0 on GoogleCloudPlatform:master.

@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Jan 31, 2015
@tseaver
Copy link
Contributor

tseaver commented Feb 3, 2015

I don't understand the motivation for making build_api_url a class method: all the callers call it via the connection instance, and then have to pass in that instance's project.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 3, 2015

The motivation doesn't precisely fit the problem. I noticed that we had an instance method here (storage) and a class method in datastore.

But the true motivation was making it to that each Connection instance only had public methods for making a single API request. Any other features would be moved elsewhere (like #583).

I'm still trying to figure out if / how build_api_url fits into that idea.

- Making Connection.make_request non-public
- Making Connection.build_api_url a class method
@dhermes dhermes force-pushed the storage-connection-tweaks branch from 53f6b86 to 9dc8773 Compare February 5, 2015 00:50
@dhermes
Copy link
Contributor Author

dhermes commented Feb 5, 2015

@tseaver Rebased on top of #580. PTAL (my new comment above).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9dc8773 on dhermes:storage-connection-tweaks into 9518db7 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Feb 5, 2015

Given that it needs instance state (the project), I can't see any benefit to making it a class method. I can even imagine that having it stay an instance method would be useful for https://github.com/GoogleCloudPlatform/gcloud-meta/issues/4

@dhermes
Copy link
Contributor Author

dhermes commented Feb 6, 2015

Cool. Closing this out.

@dhermes dhermes closed this Feb 6, 2015
@tseaver
Copy link
Contributor

tseaver commented Feb 6, 2015

FWIW, I can certainly see making both build_api_url and make_request private.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 6, 2015

@tseaver OK I'll put that in motion and send a PR.

This means build_api_url should be private in datastore as well.

How do you feel about making them private functions in the connection modules? The only reason they are attached to the Connection objects are because of the class constants which could just as easily be module constants. (Though without looking at the code it may be a PITA to import the connection module in places where a Connection object was used freely.)

@dhermes dhermes deleted the storage-connection-tweaks branch February 13, 2015 01:27
vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
…onfig (#587)

* feat: Can directly set Cloud Speech model on the SpeechToTextConfig

PiperOrigin-RevId: 482665674

Source-Link: googleapis/googleapis@64926d5

Source-Link: googleapis/googleapis-gen@351722b
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMzUxNzIyYmUxNjNkZWY3NjY2ZjEzY2I3NmIyYTI5NWQ5ZjJhODQ1MCJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this pull request Oct 21, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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 Cloud 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.

4 participants