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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java


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


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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java


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


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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * 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.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java

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

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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   *     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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java

+    }
+
    /**
     * 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java

+
    /**
     * 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java

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


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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java

+
    /**
     * 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 a review from a team as a code owner November 16, 2020 01:49

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

google-cla bot 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-label bot 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 team November 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

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreBundle.java Outdated Show resolved Hide resolved

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 master Dec 2, 2020

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

Conversation

wu-hui commented Nov 16, 2020

Uh oh!

codecov bot 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

codecov bot commented Nov 16, 2020 •

edited

Loading