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.

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

Remove CreateIfMissing option #4654

Closed
wants to merge 2 commits into from
Closed

Remove CreateIfMissing option #4654

wants to merge 2 commits into from

Conversation

chemelnucfin
Copy link
Contributor

Partial #4111

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 22, 2017
@chemelnucfin chemelnucfin force-pushed the firestore-create_if_missing branch from a5a2f5a to 3471f13 Compare December 22, 2017 04:43
dhermes
dhermes reviewed Jan 2, 2018
View reviewed changes
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

This seems mostly OK but I worry that we are replacing CreateIfMissing with Exists in places where they aren't fully equivalent.

firestore/google/cloud/firestore_v1beta1/_helpers.py
@@ -897,7 +893,7 @@ def pb_for_delete(document_path, option):
"""
write_pb = write_pb2.Write(delete=document_path)
if option is not None:
option.modify_write(write_pb, no_create_msg=NO_CREATE_ON_DELETE)
option.modify_write(write_pb)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

firestore/google/cloud/firestore_v1beta1/client.py Outdated
@@ -252,10 +251,8 @@ def write_option(**kwargs):
:meth:`~.DocumentReference.update` and
:meth:`~.DocumentReference.delete`.

Exactly one of three keyword arguments must be provided:
One of the following two keyword arguments must be provided:

This comment was marked as spam.

Sign in to view

firestore/tests/unit/test__helpers.py Outdated
@@ -1216,6 +1217,9 @@ def _helper(self, option=None, do_transform=False, **write_kwargs):
map_pb = document_pb2.MapValue(fields={
'yum': _value_pb(bytes_value=value),
})
if isinstance(option, ExistsOption):
write_kwargs = {'current_document' : {'exists': option._exists}}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@chemelnucfin chemelnucfin force-pushed the firestore-create_if_missing branch from 3471f13 to 7758dbe Compare January 3, 2018 05:38
@chemelnucfin chemelnucfin self-assigned this Jan 15, 2018
@chemelnucfin chemelnucfin added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jan 15, 2018
@chemelnucfin chemelnucfin force-pushed the firestore-create_if_missing branch from 7758dbe to bd67ea7 Compare February 3, 2018 23:21
@chemelnucfin chemelnucfin force-pushed the firestore-create_if_missing branch from bd67ea7 to 4024fcb Compare February 3, 2018 23:39
This was referenced Feb 7, 2018
Closed
Merged
@chemelnucfin chemelnucfin added the api: firestore Issues related to the Firestore API. label Feb 20, 2018
@chemelnucfin chemelnucfin changed the title Firestore: Remove CreateIfMissing option Remove CreateIfMissing option Feb 20, 2018
@chemelnucfin
Copy link
Contributor Author

Closing this pull request

The commits are in #4851 and can be reviewed commit by commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes left review comments

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees

@chemelnucfin chemelnucfin

Labels
api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chemelnucfin @dhermes @googlebot