Skip to content
This repository was archived by the owner on Mar 2, 2026. It is now read-only.

feat: add firestore bundles#319

Merged
craiglabenz merged 36 commits into
masterfrom
firestore-bundles
Mar 29, 2021
Merged

feat: add firestore bundles#319
craiglabenz merged 36 commits into
masterfrom
firestore-bundles

Conversation

@craiglabenz

@craiglabenz craiglabenz commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@craiglabenz craiglabenz self-assigned this Mar 2, 2021
@craiglabenz craiglabenz requested review from a team March 2, 2021 18:41
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the googleapis/python-firestore API. label Mar 2, 2021
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2021
@craiglabenz craiglabenz changed the title feat: add firestore bundles [wip] feat: add firestore bundles Mar 9, 2021
@craiglabenz craiglabenz changed the title feat: add firestore bundles feat: add firestore bundles [wip] Mar 9, 2021
@craiglabenz craiglabenz requested review from crwilcox and removed request for crwilcox March 11, 2021 23:08
crwilcox
crwilcox reviewed Mar 12, 2021
View reviewed changes
Comment thread google/cloud/firestore_bundle/services/__init__.py
@craiglabenz craiglabenz mentioned this pull request Mar 12, 2021
Merged
@craiglabenz craiglabenz requested a review from busunkim96 March 12, 2021 22:23
@craiglabenz craiglabenz added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 15, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 15, 2021
@craiglabenz craiglabenz requested a review from crwilcox March 15, 2021 22:45
@craiglabenz craiglabenz changed the title feat: add firestore bundles [wip] feat: add firestore bundles Mar 16, 2021
crwilcox
crwilcox reviewed Mar 19, 2021
View reviewed changes
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py
Comment thread google/cloud/firestore_v1/_helpers.py
schmidt-sebastian
schmidt-sebastian reviewed Mar 20, 2021
View reviewed changes

@schmidt-sebastian schmidt-sebastian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some comments. Before merging, can you coordinate a test with wuandy@ to make sure that these Bundles load in our clients.

Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
wu-hui
wu-hui suggested changes Mar 22, 2021
View reviewed changes
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread tests/unit/v1/test_bundle.py Outdated
wu-hui
wu-hui reviewed Mar 23, 2021
View reviewed changes

@wu-hui wu-hui left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for incorporating the comments! I think we are close now.

I do want to discuss the possibility of accepting list of documents for addNamedQuery, please let me know what do you think.

Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py
@craiglabenz

craiglabenz commented Mar 23, 2021

Copy link
Copy Markdown
Contributor Author

That's reasonable, @wu-hui. Adding now.

@craiglabenz

craiglabenz commented Mar 23, 2021

Copy link
Copy Markdown
Contributor Author

@wu-hui On second thought, accepting a list of DocumentSnapshot instances creates a new unsolved problem around how to reconstruct a proper NamedQuery object. I might vote that we defer that option for now.

@schmidt-sebastian

schmidt-sebastian commented Mar 23, 2021

Copy link
Copy Markdown

@wu-hui Can we just treat queries and documents as independent? The Python SDK could offer APIs to add named queries and documents, but we don't necessarily need to associate the documents with the queries. The Mobile clients should still be able to handle this data (correct me if I am wrong).

I'm also very surprised that Python still does not have QuerySnapshots and a bit scared to find out what this means for the Watch API :)

@wu-hui

wu-hui commented Mar 23, 2021

Copy link
Copy Markdown

@wu-hui Can we just treat queries and documents as independent? The Python SDK could offer APIs to add named queries and documents, but we don't necessarily need to associate the documents with the queries. The Mobile clients should still be able to handle this data (correct me if I am wrong).

I'm also very surprised that Python still does not have QuerySnapshots and a bit scared to find out what this means for the Watch API :)

I think we might have to accept Query and run on behalf of user for Python, there is no other way to get StructuredQuery otherwise. WDYT?

wu-hui
wu-hui approved these changes Mar 25, 2021
View reviewed changes

@wu-hui wu-hui left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM with some nits on comments

Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
Comment thread google/cloud/firestore_bundle/bundle.py Outdated
@wu-hui

wu-hui commented Mar 29, 2021

Copy link
Copy Markdown

LGTM!

@craiglabenz craiglabenz merged commit c54de50 into master Mar 29, 2021
@craiglabenz craiglabenz deleted the firestore-bundles branch March 29, 2021 20:21
@failure-checker failure-checker Bot mentioned this pull request Mar 30, 2021
Closed
@tseaver tseaver mentioned this pull request Jan 13, 2026
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@busunkim96 busunkim96 Awaiting requested review from busunkim96
@tswast tswast Awaiting requested review from tswast
3 more reviewers
@schmidt-sebastian schmidt-sebastian schmidt-sebastian left review comments
@crwilcox crwilcox crwilcox approved these changes
@wu-hui wu-hui wu-hui approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@craiglabenz craiglabenz

Labels

api: firestore Issues related to the googleapis/python-firestore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@craiglabenz @crwilcox @schmidt-sebastian @wu-hui @yoshi-kokoro @craiglabenz-g