Skip to content

Requiring Connection.lookup to take a list in datastore. #582

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

Merged
merged 1 commit into from
Feb 3, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jan 31, 2015

This is in line with our decision in api.get / api.put / api.delete to only take a list and not over-use isinstance().

This is in line with our decision in api.get / api.put / api.delete
to only take a list and not over-use isinstance().
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2015
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Jan 31, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 57e11b1 on dhermes:connection-lookup-only-list into d858ff0 on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 31, 2015

@tseaver I came across this while checking out our two Connection classes while trying to smooth off the surface for storage. ISTM that methods in a Connection should have no side effects and that extra work under the covers should happen elsewhere (e.g. in the Bucket instance methods or in free functions like we define in datastore.api.)

I propose that each public method in a Connection should do something concrete (e.g. build a URL) or make a single HTTP API request with no side effects. Currently Connection.lookup (in datastore) breaks this behavior.

How do you feel about moving the "do a lookup until all deferred are returned" behavior into api.get (or a helper in the api module).

@tseaver
Copy link
Contributor

tseaver commented Feb 3, 2015

This PR LGTM.

I'm fine with moving the DWIM bits out of Connection, too.

dhermes added a commit that referenced this pull request Feb 3, 2015
Requiring Connection.lookup to take a list in datastore.
@dhermes dhermes merged commit f854cd1 into googleapis:master Feb 3, 2015
@dhermes dhermes deleted the connection-lookup-only-list branch February 3, 2015 17:33
@dhermes
Copy link
Contributor Author

dhermes commented Feb 3, 2015

Great. DWIM implemented in #583

vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
…eation metadata (#582)

- [ ] Regenerate this pull request now.

docs: clarify SuggestionFeature enums which are specific to chat agents

PiperOrigin-RevId: 478522249

Source-Link: https://togithub.com/googleapis/googleapis/commit/8bd89cd4fc964360198362ef49c72ef90543bf45

Source-Link: https://togithub.com/googleapis/googleapis-gen/commit/ddf381e8fcebbdde902df0419b30908d01c63e0e
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZGRmMzgxZThmY2ViYmRkZTkwMmRmMDQxOWIzMDkwOGQwMWM2M2UwZSJ9
parthea pushed a commit that referenced this pull request Oct 21, 2023
Source-Link: https://togithub.com/googleapis/synthtool/commit/0ddbff8012e47cde4462fe3f9feab01fbc4cdfd6
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:bced5ca77c4dda0fd2f5d845d4035fc3c5d3d6b81f245246a36aee114970082b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore 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