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

fix: repair implementation of Client.reserve_ids #76

Merged
merged 2 commits into from
Sep 2, 2020

Conversation

chrisrossi
Copy link
Contributor

Client.reserve_ids has been reimplemented in a way that should be a lot more
useful, and has been renamed Client_reserve_ids_sequential, leaving
the old name as a deprecated alias.

Client.reserve_ids_multi has been added, which takes sequence of
complete keys to reserve.

Fixes #37

`Client.reserve_ids` has been reimplemented in a way that should be a lot more
useful, and has been renamed `Client_reserve_ids_sequential`, leaving
the old name as a deprecated alias.

`Client.reserve_ids_multi` has been added, which takes sequence of
complete keys to reserve.

Fixes googleapis#37
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 25, 2020
@@ -1252,38 +1394,64 @@ class _Entity(dict):


class _Key(object):
_MARKER = object()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reserve_ids_sequential required a mock Key class that was closer to the real thing.

@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/python-datastore API. label Aug 26, 2020

Please use either :meth:`reserve_ids_multi` (recommended) or
:meth:`reserve_ids_sequential`.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth a DeprecationWarningMessage here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# Smoke test to make sure it doesn't blow up. No return value or
# verifiable side effect to verify.
keys = [Config.CLIENT.key("KIND", 1234), Config.CLIENT.key("KIND", 1235)]
Config.CLIENT.reserve_ids_multi(keys)
Copy link
Contributor

Choose a reason for hiding this comment

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

Prolly needs a test for reserve_ids, including a with warnings.catch_warnings(log=True) as warned: wrapper (assuming you agree with me above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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


kwargs = _make_retry_timeout_kwargs(retry, timeout)
key_pbs = [key.to_protobuf() for key in complete_keys]
self._datastore_api.reserve_ids(complete_keys[0].project, key_pbs, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a concern that we aren't verifying that the keys match on things like project? We assume the first key represents the others right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that. That is the assumption. Obviously the assumption might be wrong. But I figured the API would probably kick it back if it was. I can verify if that is or isn't the case. Or do the check here, regardless, if that makes us more comfortable.

Copy link
Contributor Author

@chrisrossi chrisrossi Aug 28, 2020

Choose a reason for hiding this comment

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

@crwilcox I've verified that the API will kick back an InvalidArgument error in this case. If it's just up to me I'd probably just leave it at that. If you think there should be a check in the client, though, let me know.

@chrisrossi
Copy link
Contributor Author

@tseaver Can you take a look when you get a chance?

@tseaver tseaver changed the title fix: fix implementation of Client.reserve_ids fix: repair implementation of Client.reserve_ids Sep 2, 2020
@tseaver tseaver merged commit 7df727d into googleapis:master Sep 2, 2020
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 googleapis/python-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.

Reserve_ids documentation has a typo, and implementation seems bugged
3 participants