Skip to content

Remove CreateIfMissing option #4654

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
Closed
chemelnucfin wants to merge 2 commits into from
Closed

Remove CreateIfMissing option #4654

chemelnucfin wants to merge 2 commits into from

Conversation

@chemelnucfin
Copy link
Contributor

@chemelnucfin chemelnucfin commented Dec 22, 2017

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
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
: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
'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

chemelnucfin commented Feb 22, 2018

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

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

1 more reviewer

@dhermes dhermes dhermes left review comments

Reviewers whose approvals may not affect merge requirements

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