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

feat: add firestore bundles #319

Merged
merged 36 commits into from
Mar 29, 2021
Merged

feat: add firestore bundles #319

merged 36 commits into from
Mar 29, 2021

Conversation

craiglabenz
Copy link
Contributor

No description provided.

@craiglabenz craiglabenz self-assigned this Mar 2, 2021
@craiglabenz craiglabenz requested a review from a team March 2, 2021 18:41
@craiglabenz craiglabenz requested a review from a team as a code owner 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
@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
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Show resolved Hide resolved
google/cloud/firestore_v1/_helpers.py Show resolved Hide resolved
schmidt-sebastian
schmidt-sebastian reviewed Mar 20, 2021
View reviewed changes
Copy link

@schmidt-sebastian schmidt-sebastian left a comment

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.

google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
wu-hui
wu-hui requested changes Mar 22, 2021
View reviewed changes
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
tests/unit/v1/test_bundle.py Outdated Show resolved Hide resolved
wu-hui
wu-hui reviewed Mar 23, 2021
View reviewed changes
Copy link

@wu-hui wu-hui left a comment

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.

google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Show resolved Hide resolved
@craiglabenz
Copy link
Contributor Author

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

@craiglabenz
Copy link
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
Copy link

@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
Copy link

wu-hui commented Mar 23, 2021

@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
Copy link

@wu-hui wu-hui left a comment

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

google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
google/cloud/firestore_bundle/bundle.py Outdated Show resolved Hide resolved
@wu-hui
Copy link

wu-hui commented Mar 29, 2021

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 Dec 9, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schmidt-sebastian schmidt-sebastian schmidt-sebastian left review comments

@wu-hui wu-hui wu-hui approved these changes

@crwilcox crwilcox crwilcox approved these changes

@busunkim96 busunkim96 Awaiting requested review from busunkim96

@tswast tswast Awaiting requested review from tswast

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