Skip to content

Conversation

@rafilong
Copy link
Contributor

@rafilong rafilong commented Aug 7, 2020

Add snippets for new async Firestore interface. Snippets are ported from firestore/cloud-client to firestore/cloud-async-client.

@rafilong rafilong requested a review from a team as a code owner August 7, 2020 17:55
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2020
@crwilcox crwilcox requested a review from BenWhitehead August 7, 2020 17:57
@rafilong rafilong force-pushed the firestore-async-snippets branch from f2def9e to 495ae79 Compare August 7, 2020 18:07
@rafilong
Copy link
Contributor Author

rafilong commented Aug 7, 2020

Force push to overwrite history and make review easier. 6fb670d directly copies sync snippets to async directory, and 495ae79 converts to async. Would recommend reviewing the latter for snippets*.py.

@rafilong
Copy link
Contributor Author

rafilong commented Aug 7, 2020

Question: are annotations scoped to the repo, project, or file? I assumed the latter, so snippets tags are shared between sync and async.

Copy link
Contributor

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

This looks great @rafilong!!! Thanks for getting all this done.

One thing we need to do is figure out a naming scheme for the async samples so that defsite doesn't union these new samples and the existing ones.

@busunkim96 @crwilcox Do we have a naming schema already defined for async clients and samples?

@BenWhitehead BenWhitehead added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 10, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 10, 2020
@crwilcox
Copy link
Contributor

We don't have a pattern yet but I feel like appending async to the region tag makes sense as it would group with the other sample. Still haven't settled on how to display these though. For instance, should they both show under python or do we want a new Python (async) tab similar to how Java has the Android version.

Either way we need to settle on a new region tag. How are the Android ones separated?

@BenWhitehead
Copy link
Contributor

It looks like in Android the java/kotlin separation is handled by the sample registry indirection (though I'm not familiar with how that actually works).

I'm okay with having an _async suffix if that's how we want to do it. Though, I'm not sure of the best way to do this. I know lots of snippets are managed lots of different ways.

@rafilong
Copy link
Contributor Author

@BenWhitehead @crwilcox would adding an _async suffix as a temporary stopgap so we can merge this in reasonable? It should be a relatively quick fix in the future to implement a better solution, and a suffix seems fairly reasonable.

@BenWhitehead
Copy link
Contributor

Adding the _async suffix SGTM

@rafilong
Copy link
Contributor Author

@BenWhitehead @crwilcox suffix added!

@crwilcox crwilcox added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 12, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 12, 2020
@crwilcox crwilcox merged commit 86dbbb5 into GoogleCloudPlatform:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants