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

Updating datastore HTTP wrapper. #4388

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Nov 13, 2017

  • Making the method arguments (both in position and name) match the arguments for google.cloud.datastore_v1.gapic.datastore_client.DatastoreClient
  • Passing positional as positional and keyword as keyword when using the low-level API client in Client.get() (was previously using all keyword arguments to .lookup(), which caused this issue)
  • Updating mock call assertions to match the change in calling behavior.

Fixes #4387.

- Making the method arguments (both in position and name) match the
  arguments for `google.cloud.datastore_v1.gapic.datastore_client.DatastoreClient`
- Passing positional as positional and keyword as keyword when using
  the low-level API client in `Client.get()` (was previously using all
  keyword arguments to `.lookup()`, which caused this issue)
- Updating mock call assertions to match the change in calling behavior.

Fixes googleapis#4387.
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Nov 13, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 13, 2017
@dhermes
Copy link
Contributor Author

dhermes commented Nov 13, 2017

When I checked the datastore_client.DatastoreClient interface, this also revealed

  • Our low-level HTTP client doesn't have retry= or timeout= keywords (@jonparrott any good ideas here?)
  • Our low-level HTTP client doesn't support the reserve_ids method

Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Ugh, it feeds a little B&D / un-Pythonic to insist on "positional-only" args. Having the underlying bits change their argument names is actually a bit of a foul. I guess this is "not user-facing", so we can go ahead.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 13, 2017

@tseaver I think it's "good form" to do. With the addition of keyword-only arguments in Python 3, I'm not so sure that's a common attitude. From my perspective, using positional as positional and keyword as keyword actually indicates that the knows that.

Also, this isn't the first time we've been bitten by the auto-generated interface changing, which is why I advocate for "strict" adherence the offered interface.

PS What's B&D?

@dhermes dhermes merged commit a105bba into googleapis:master Nov 13, 2017
@dhermes dhermes deleted the fix-4387 branch November 13, 2017 18:15
@tseaver
Copy link
Contributor

tseaver commented Nov 13, 2017

@dhermes note that I approved the PR: I'm just "arguing with the ref."

Also, this isn't the first time we've been bitten by the auto-generated interface changing, which is why I advocate for "strict" adherence the offered interface.

They are equally capable of breaking this usage (changing order of "positional" arguments, for instance, which would be covered by using only keyword args when calling them).

B&D -> "bondage-and-discipline".

chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 15, 2017
- Making the method arguments (both in position and name) match the
  arguments for `google.cloud.datastore_v1.gapic.datastore_client.DatastoreClient`
- Passing positional as positional and keyword as keyword when using
  the low-level API client in `Client.get()` (was previously using all
  keyword arguments to `.lookup()`, which caused this issue)
- Updating mock call assertions to match the change in calling behavior.

Fixes googleapis#4387.
chemelnucfin pushed a commit to chemelnucfin/google-cloud-python that referenced this pull request Nov 15, 2017
- Making the method arguments (both in position and name) match the
  arguments for `google.cloud.datastore_v1.gapic.datastore_client.DatastoreClient`
- Passing positional as positional and keyword as keyword when using
  the low-level API client in `Client.get()` (was previously using all
  keyword arguments to `.lookup()`, which caused this issue)
- Updating mock call assertions to match the change in calling behavior.

Fixes googleapis#4387.
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