fix: repair implementation of Client.reserve_ids
#76
Conversation
`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
bot
added
the
cla: yes
| @@ -1252,38 +1394,64 @@ class _Entity(dict): | |||
|
|
|||
|
|
|||
| class _Key(object): | |||
| _MARKER = object() | |||
There was a problem hiding this comment.
reserve_ids_sequential required a mock Key class that was closer to the real thing.
product-auto-label
bot
added
the
api: datastore
|
|
||
| Please use either :meth:`reserve_ids_multi` (recommended) or | ||
| :meth:`reserve_ids_sequential`. | ||
| """ |
There was a problem hiding this comment.
Worth a DeprecationWarningMessage here?
| # 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) |
There was a problem hiding this comment.
Prolly needs a test for reserve_ids, including a with warnings.catch_warnings(log=True) as warned: wrapper (assuming you agree with me above).
|
|
||
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
@tseaver Can you take a look when you get a chance? |
tseaver
changed the title
Client.reserve_idsClient.reserve_ids
Client.reserve_idshas been reimplemented in a way that should be a lot moreuseful, and has been renamed
Client_reserve_ids_sequential, leavingthe old name as a deprecated alias.
Client.reserve_ids_multihas been added, which takes sequence ofcomplete keys to reserve.
Fixes #37