Skip to content

New Datastore auto-gen. #4348

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
lukesneeringer merged 3 commits into from
Nov 8, 2017
Merged

New Datastore auto-gen. #4348

lukesneeringer merged 3 commits into from
Nov 8, 2017

Conversation

@lukesneeringer
Copy link
Contributor

@lukesneeringer lukesneeringer commented Nov 6, 2017

This updates the auto-gen layer for Datastore.

@lukesneeringer lukesneeringer self-assigned this Nov 6, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2017
@dhermes
Copy link
Contributor

dhermes commented Nov 6, 2017

@lukesneeringer The dependency on gapic-google-cloud-datastore-v1 in datastore/setup.py should go away as well.

@chemelnucfin chemelnucfin mentioned this pull request Nov 6, 2017
Closed
@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Nov 6, 2017

Pretty sure it did go away. I just double-checked. Did I miss something?

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Nov 6, 2017

@dhermes I am also a bit baffled by the new system test errors. Any idea what Value must be an iterable in lookup is all about? (I can investigate if you do not know.)

dhermes
dhermes reviewed Nov 6, 2017
View reviewed changes
datastore/setup.py
'google-auth >= 1.0.2, < 2.0dev',
'google-gax >= 0.15.15, < 0.16dev',
'googleapis-common-protos[grpc] >= 1.5.2, < 2.0dev',
'requests >= 2.18.4, < 3.0dev',

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Nov 6, 2017

Pretty sure it did go away. I just double-checked. Did I miss something?

No, I was mistaken (I missed setup.py in the diff).

I am also a bit baffled by the new system test errors...

I do not know, but it feels like a mock doesn't have __iter__ capabilities.

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Nov 6, 2017

I do not know, but it feels like a mock doesn't have __iter__ capabilities.

It is a system test, so it should not be a mock issue. I am asking Alan to look into it for a bit.

@chemelnucfin
Copy link
Contributor

chemelnucfin commented Nov 7, 2017

So the issue is that in datastore_client.py lookup takes positional arguments, (project_id, keys, read_options=None), and in _extended_lookup from client.py, datastore_api.lookup takes (*args, **kwargs). if those calls mismatch, we get problems.

Changing the datastore_client.py lookup into def lookup(self,
project,
read_options=None,
keys=None,
works. There are other options that can be changed, but the others will break tests.
Protobuf index order is project, options, and then keys if that makes a difference.

Any comments?

@chemelnucfin chemelnucfin mentioned this pull request Nov 8, 2017
Merged
@chemelnucfin
Copy link
Contributor

chemelnucfin commented Nov 8, 2017

@lukesneeringer

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Nov 8, 2017

Thanks -- that diagnosis was perfect. Fixed it.

@dhermes
Copy link
Contributor

dhermes commented Nov 8, 2017

Now there's a linter feature request! Always make sure we pass positional as positional and keyword as keyword.

@lukesneeringer
Copy link
Contributor Author

lukesneeringer commented Nov 8, 2017

Unrelated CI failure, and tests are known to work. Merging.

@lukesneeringer lukesneeringer merged commit fde9109 into master Nov 8, 2017
@lukesneeringer lukesneeringer deleted the datastore-gapic branch November 8, 2017 22:15
@mathewcohle
Copy link

mathewcohle commented Nov 13, 2017 •
edited
Loading

Commit 9246de7 introduced following bug:

  File "/usr/local/lib/python2.7/dist-packages/google/cloud/datastore/client.py", line 321, in get
    eventual=eventual)
  File "/usr/local/lib/python2.7/dist-packages/google/cloud/datastore/client.py", line 376, in get_multi
    transaction_id=transaction and transaction.id,
  File "/usr/local/lib/python2.7/dist-packages/google/cloud/datastore/client.py", line 138, in _extended_lookup
    keys=key_pbs,
TypeError: lookup() got an unexpected keyword argument 'project_id'

Patch

From 7470329ecf13d9f85bfe8af71ee533856ff9ec4c Mon Sep 17 00:00:00 2001
From: mathewcohle <mathewcohle@gmail.com>
Date: Mon, 13 Nov 2017 11:41:01 +0100
Subject: [PATCH] Fix wrong keyword argument name introduced in #4348

---
 datastore/google/cloud/datastore/client.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/datastore/google/cloud/datastore/client.py b/datastore/google/cloud/datastore/client.py
index a3d1f9d43..c42c75e32 100644
--- a/datastore/google/cloud/datastore/client.py
+++ b/datastore/google/cloud/datastore/client.py
@@ -133,7 +133,7 @@ def _extended_lookup(datastore_api, project, key_pbs,
     while loop_num < _MAX_LOOPS:  # loop against possible deferred.
         loop_num += 1
         lookup_response = datastore_api.lookup(
-            project_id=project,
+            project=project,
             read_options=read_options,
             keys=key_pbs,
         )
-- 
2.15.0

Info

$ pip freeze | grep google

gapic-google-cloud-error-reporting-v1beta1==0.15.3
gapic-google-cloud-logging-v2==0.91.3
gax-google-logging-v2==0.8.3
gax-google-pubsub-v1==0.8.3
google-api-core==0.1.1
google-api-python-client==1.6.4
google-auth==1.2.0
google-cloud==0.30.1.dev1
google-cloud-bigquery==0.28.0
google-cloud-bigtable==0.28.1
google-cloud-core==0.28.0
google-cloud-datastore==1.4.1.dev1
google-cloud-dns==0.28.0
google-cloud-error-reporting==0.28.0
google-cloud-firestore==0.28.0
google-cloud-language==1.0.0
google-cloud-logging==1.4.0
google-cloud-monitoring==0.28.0
google-cloud-pubsub==0.29.0
google-cloud-resource-manager==0.28.0
google-cloud-runtimeconfig==0.28.0
google-cloud-spanner==0.29.0
google-cloud-speech==0.30.0
google-cloud-storage==1.6.0
google-cloud-trace==0.16.0
google-cloud-translate==1.3.0
google-cloud-videointelligence==0.28.0
google-cloud-vision==0.28.0
google-gax==0.12.5
google-resumable-media==0.3.1
googleapis-common-protos==1.5.3
grpc-google-iam-v1==0.11.4
grpc-google-logging-v2==0.8.1
grpc-google-pubsub-v1==0.8.1
proto-google-cloud-error-reporting-v1beta1==0.15.3
proto-google-cloud-logging-v2==0.91.3

@lukesneeringer

parthea pushed a commit that referenced this pull request Nov 24, 2025
@lukesneeringer
New Datastore auto-gen. (#4348)
5842892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dhermes dhermes dhermes left review comments

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukesneeringer lukesneeringer

Labels

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.

6 participants

@lukesneeringer @dhermes @chemelnucfin @mathewcohle @tseaver @googlebot