feat: Release bundles by wu-hui · Pull Request #466 · googleapis/java-firestore

wu-hui · 2020-11-16T01:49:58Z

PR to release bundles. Will merge when we decide to release.

codecov · 2020-11-16T01:55:29Z

Codecov Report

Merging #466 (d0588f8) into master (ce70619) will decrease coverage by 0.05%.
The diff coverage is 20.00%.

@@ Coverage Diff @@## master #466 +/- ##
============================================
- Coverage 73.40% 73.35% -0.06% + Complexity 1095 1086 -9 
============================================
Files 65 65 Lines 5786 5790 +4 Branches 721 723 +2 ============================================
Hits 4247 4247 - Misses 1313 1317 +4 
Partials 226 226

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/cloud/firestore/FirestoreImpl.java	`75.91% <0.00%> (-1.70%)`	`27.00 <0.00> (ø)`
...va/com/google/cloud/firestore/FirestoreBundle.java	`92.45% <50.00%> (-0.89%)`	`3.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce70619...d0588f8. Read the comment docs.

schmidt-sebastian · 2020-11-16T16:02:58Z


+ /**
+ * Returns a FirestoreBundle.Builder {@link FirestoreBundle.Builder} instance for the given bundle
+ * Id.


schmidt-sebastian · 2020-11-16T16:03:28Z

+ * Id.
+ *
+ * @param bundleId If null, an random string will be used for the Id. The Id is used by the client
+ * SDKs loading bundles to identify if a bundle has been loaded before.


s/Id/ID and swap both sentences.
If a null bundle ID is how me mainly expect the bundles to be used, should we add an overload that doesn't require an explicit null?

Yep, that is better.
I also realized we provided no way for the client to get the bundle id if it is auto-id, they probably want to use it to manage their bundle serving. I added a getId to builder. Will pass a note to the council again.

Thought on adding bundleBuilder() with no arguments? I believe this would be the main way to interact with bundles, and we should make it as easy as possible.

BenWhitehead · 2020-11-16T20:23:41Z

New methods on the Firestore interface will need ignore rules added to google-cloud-firestore/clirr-ignored-differences.xml. An example of how to allow a new method on the interface can be see in this rule.

wu-hui

Also add to clirr ignore file.

wu-hui · 2020-11-17T15:26:45Z


+ /**
+ * Returns a FirestoreBundle.Builder {@link FirestoreBundle.Builder} instance for the given bundle
+ * Id.


wu-hui · 2020-11-17T15:28:52Z

+ * Id.
+ *
+ * @param bundleId If null, an random string will be used for the Id. The Id is used by the client
+ * SDKs loading bundles to identify if a bundle has been loaded before.


Yep, that is better.
I also realized we provided no way for the client to get the bundle id if it is auto-id, they probably want to use it to manage their bundle serving. I added a getId to builder. Will pass a note to the council again.

…e/FirestoreBundle.java Co-authored-by: yoshi-code-bot <70984784+yoshi-code-bot@users.noreply.github.com>

schmidt-sebastian · 2020-11-17T16:12:46Z

+ * ID.
+ *
+ * @param bundleId The ID is used by the client SDKs loading bundles to identify if a bundle has
+ * been loaded before. If null, an random string will be used for the ID.


schmidt-sebastian · 2020-11-17T16:14:47Z

+ * Id.
+ *
+ * @param bundleId If null, an random string will be used for the Id. The Id is used by the client
+ * SDKs loading bundles to identify if a bundle has been loaded before.


Thought on adding bundleBuilder() with no arguments? I believe this would be the main way to interact with bundles, and we should make it as easy as possible.

schmidt-sebastian · 2020-11-17T22:00:51Z

@wu-hui What are your thoughts on the overload that doesn't take a bundle ID?

wu-hui · 2020-11-17T22:06:38Z

@wu-hui What are your thoughts on the overload that doesn't take a bundle ID?

Sorry I meant to reply your earlier comment.

I think it is added (https://github.com/googleapis/java-firestore/pull/466/files#diff-b9277c291d91b6930b9a93135a3090afa8aebdacc3c728dec58f858db2bc7b95R177), were you looking at an older commit?

schmidt-sebastian

LGTM. Sorry I must have looked at a different commit.

schmidt-sebastian · 2020-11-17T22:11:15Z

+ * been loaded before. If null, a random string will be used for the ID.
+ */
+ @Nonnull
+ FirestoreBundle.Builder bundleBuilder(@Nullable String bundleId);


s/@Nullable/@NotNull

markarndt

@wu-hui .
I think the Node version has a clearer definition of bundleID: "The id of the bundle. When loaded on clients, client SDKs use this id and the timestamp associated with the built bundle to tell if it has been loaded already. If not specified, a random identifier will be used."

markarndt · 2020-11-19T19:49:13Z

+ }
+
 /**
 * Adds a Firestore document snapshot to the bundle. Both the documents data and the document


Remove plural "Both the document data and...."

markarndt · 2020-11-19T19:50:53Z

+
 /**
 * Adds a Firestore document snapshot to the bundle. Both the documents data and the document
 * read time will be included in the bundle.


Line 124-129 below, for add() doc, remove plural so "Adds a Firestore query snapshot to..."

wu-hui

Thanks Mark, I updated the comment for bundle ID.

wu-hui · 2020-11-27T15:03:11Z

+ }
+
 /**
 * Adds a Firestore document snapshot to the bundle. Both the documents data and the document


wu-hui · 2020-11-27T15:04:58Z

+
 /**
 * Adds a Firestore document snapshot to the bundle. Both the documents data and the document
 * read time will be included in the bundle.


feat: Release bundles
3d190ec

wu-hui requested review from a teamNovember 16, 2020 01:49

google-claBot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2020

wu-hui requested a review from BenWhitehead November 16, 2020 01:50

wu-hui assigned BenWhitehead Nov 16, 2020

product-auto-labelBot added the api: firestore Issues related to the googleapis/java-firestore API. label Nov 16, 2020

schmidt-sebastian reviewed Nov 16, 2020
View reviewed changes

Add clirr ignore
a28d9eb

wu-hui requested a review from a teamNovember 17, 2020 15:17

Add overload and allow user get id back.
f115b40

wu-hui commented Nov 17, 2020
View reviewed changes

wu-hui assigned schmidt-sebastian Nov 17, 2020

yoshi-code-bot reviewed Nov 17, 2020
View reviewed changes

Comment threadgoogle-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java Outdated

Update google-cloud-firestore/src/main/java/com/google/cloud/firestor…
f42625b
…e/FirestoreBundle.java Co-authored-by: yoshi-code-bot <70984784+yoshi-code-bot@users.noreply.github.com>

schmidt-sebastian reviewed Nov 17, 2020
View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 17, 2020

Fix grammar
fe05c46

wu-hui assigned schmidt-sebastian and unassigned wu-hui Nov 17, 2020

schmidt-sebastian assigned wu-hui and unassigned BenWhitehead and schmidt-sebastian Nov 17, 2020

schmidt-sebastian approved these changes Nov 17, 2020
View reviewed changes

schmidt-sebastian self-assigned this Nov 17, 2020

Nullable to Nonnull
06eae54

BenWhitehead approved these changes Nov 18, 2020
View reviewed changes

markarndt reviewed Nov 19, 2020
View reviewed changes

Addressing documentation comments.
d0588f8

wu-hui commented Nov 27, 2020
View reviewed changes

wu-hui merged commit 3af065e into masterDec 2, 2020

wu-hui deleted the wuandy/ReleaseBundles branch December 2, 2020 19:47

Uh oh!

Conversation

wu-hui commented Nov 16, 2020

Uh oh!

codecovBot commented Nov 16, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenWhitehead commented Nov 16, 2020

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Nov 17, 2020

Uh oh!

wu-hui commented Nov 17, 2020

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markarndt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecovBot commented Nov 16, 2020•
edited
Loading