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

feat: support autoconversion of Entity to Key for purposes of delete & delete_multi #123

Merged
merged 7 commits into from
Dec 4, 2020

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Nov 26, 2020

Fixes #122

Can discuss if we should

  • have a warning
    or
  • modify documented type of delete and delete_multi to mention they can take Entity
❯ python tests/system/test_delete.py
Deleting 20 entities: 5121006972698624, 5150910414061568, 5629847317512192, 5635426882682880, 5644748102565888, 5660593176444928, 5670520389369856, 5680769993277440, 5693029306335232, 5697954694299648, 5701878616686592, 5708050082037760, 5709769612460032, 5744781917421568, 5746397093560320, 5746853299617792, 5754048175144960, 5756417252261888, 5762132310228992, 6251525647106048
/Users/crwilcox/workspace/python-datastore/google/cloud/datastore/client.py:682: UserWarning: A list of :class:`google.cloud.datastore.key.Key` is expected. Extracting Key from :class:`google.cloud.datastore.entity.Entity`.
  warnings.warn(message, UserWarning)

@crwilcox crwilcox requested a review from a team November 26, 2020 00:27
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/python-datastore API. label Nov 26, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 26, 2020
Copy link

@wescpy wescpy left a comment

Choose a reason for hiding this comment

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

LGTM

+ ":class:`google.cloud.datastore.entity.Entity`."
)
warnings.warn(message, UserWarning)
key = key.key
Copy link

Choose a reason for hiding this comment

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

  • [L683] This works but may result in occasional snarky comments. I would not, however, rewrite it as key = getattr(key, 'key') cuz that's worse.
  • [L646] Would any changes be req'd in this part of the docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may result in occasional snarky comments.

@wescpy I don't quite understand. Could you elaborate.

As for docstr, that is part of the open conversation. We can either

  1. make this a stealth change to help smooth migration. As written with the warning, I wouldn't want to add to docs
  2. say this is actually supported, in which case, no warning should happen, but we should specify in types you can hand an entity as well.

I would be fine to call this supported, but started from the 'smoothing' over as lightest touch.

Copy link

@wescpy wescpy Dec 1, 2020

Choose a reason for hiding this comment

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

Snarky meaning some people may criticize naming choice in key = key.key while it's perfectly valid IMO. Anyway, it was an aside and a mostly irrelevant comment... pls ignore. For the other one, I'm leaning #2. I'd also like to hear from members of the community as well, esp. those who work w/Datastore regularly. In particular, I'd like to know whether this is something they find useful.

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.

Needs unit tests for this feature.

google/cloud/datastore/client.py Outdated Show resolved Hide resolved
Co-authored-by: Tres Seaver <tseaver@palladion.com>
@google-cla
Copy link

google-cla bot commented Nov 30, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 30, 2020
@crwilcox
Copy link
Contributor Author

@tseaver sort of waiting until we agree if to go with full support or sneaky support. Do you have an opinion? I am not terrified about adding type coercion myself, and this will make migration from NDB a bit smoother.

@tseaver
Copy link
Contributor

tseaver commented Dec 2, 2020

@crwilcox I would actually go with full support (no warning, update the docstring).

@google-cla
Copy link

google-cla bot commented Dec 4, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Dec 4, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Dec 4, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Dec 4, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Dec 4, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@crwilcox crwilcox added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 4, 2020
@crwilcox crwilcox dismissed tseaver’s stale review December 4, 2020 23:19

feedback addressed. LGTM elsewhere ;)

Copy link

@dmahugh dmahugh left a comment

Choose a reason for hiding this comment

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

LGTM

@dmahugh dmahugh merged commit bf1dde6 into googleapis:master Dec 4, 2020
Copy link

@engelke engelke left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Allow passing Entity types to delete_multi in addition to Key types.
5 participants