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.

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

Move 'datastore.api' functions to 'datastore.client.Client' methods. #963

Merged
merged 4 commits into from
Jul 3, 2015
Merged

Move 'datastore.api' functions to 'datastore.client.Client' methods. #963

merged 4 commits into from
Jul 3, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 3, 2015

Remove the 'datastore.api' module altogether.

See #944.

@tseaver
Move 'datastore.api' functions to d'atastore.client.Client 'methods.
5d08a76 
Remove the 'datastore.api' module altogether.

See #944.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 3, 2015
@tseaver tseaver mentioned this pull request Jul 3, 2015
Closed
8 tasks
@dhermes dhermes changed the title Move 'datastore.api' functions to d'atastore.client.Client 'methods. Move 'datastore.api' functions to 'datastore.client.Client 'methods. Jul 3, 2015
@dhermes dhermes changed the title Move 'datastore.api' functions to 'datastore.client.Client 'methods. Move 'datastore.api' functions to 'datastore.client.Client' methods. Jul 3, 2015
dhermes
dhermes reviewed Jul 3, 2015
View reviewed changes
gcloud/datastore/client.py
eventual=False, transaction_id=None):
"""Repeat lookup until all keys found (unless stop requested).

Helper function for :method:`Client.get_multi`.

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Jul 3, 2015

I did a full review of everything except test_client.py. Any suggestions for how to review?

Also, _require_dataset_id and _require_connection disappeared from api.py. Is this a problem? I suppose _require_dataset_id has been covered partially by the constructor of Client, but _require_connection not really at all (until the AttributeError when the user tries to use the connection None as a Connection).

@tseaver
Copy link
Contributor Author

tseaver commented Jul 3, 2015

Any suggestions for how to review test_client.py?

Coverage is still 100%, and I moved the tests which didnt exercise "implicit" dataset / connection over mostly wholesale.

_require_dataset_id and _require_connection disappeared from api.py. Is this a problem?

They were only there to support handling implicit connection / dataset ID for the API methods. We do exercise creating a Client instance with no arguments, which should be sufficient AFAICT.

dhermes
dhermes reviewed Jul 3, 2015
View reviewed changes
gcloud/datastore/client.py
if not keys:
return []

ids = list(set([key.dataset_id for key in keys]))

This comment was marked as spam.

Sign in to view

@tseaver tseaver added the api: datastore Issues related to the Datastore API. label Jul 3, 2015
@dhermes
Copy link
Contributor

dhermes commented Jul 3, 2015

All right, looks like the last remaining bit is about the raises section of put_multi.

I'd like the "It seems we should be more consistent (not in this PR) about how we do this." to be addressed at some point, but not here (i.e. just raising in _multi when we don't get a list and maybe vice-versa in the singleton methods)

@dhermes
Copy link
Contributor

dhermes commented Jul 3, 2015

Not sure what ugh was directed at but LGTM 😄

tseaver added a commit that referenced this pull request Jul 3, 2015
@tseaver
Merge pull request #963 from tseaver/944-move_api_functions_to_client…
675c839 
…_methods

Move 'datastore.api' functions to 'datastore.client.Client' methods.
@tseaver tseaver merged commit 675c839 into googleapis:master Jul 3, 2015
@tseaver tseaver deleted the 944-move_api_functions_to_client_methods branch July 3, 2015 20:40
@dhermes dhermes mentioned this pull request Jul 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: datastore Issues related to the Datastore 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.
3 participants
@tseaver @dhermes @googlebot