Skip to content

Migrate transforms to use Values #1205

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
Merged
merged 11 commits into from
Feb 6, 2020
Merged

Migrate transforms to use Values #1205

merged 11 commits into from
Feb 6, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes Override cla label Feb 5, 2020
@schmidt-sebastian
Copy link
Contributor Author

/test smoke-tests

@codecov
Copy link

codecov bot commented Feb 5, 2020 •
edited
Loading

Codecov Report

Merging #1205 into mrschmidt/rewritefieldvalue will increase coverage by 0.01%.
The diff coverage is 100%.

Flag Coverage Δ Complexity Δ
#FirebaseFirestore 62.17% <100%> (+0.01%) 2263 <9> (+2) ⬆️
#FirebaseFirestore_Ktx 41.17% <ø> (ø) 0 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...l/mutation/NumericIncrementTransformOperation.java 78.94% <ø> (+3.94%) 15 <0> (-3) ⬇️
...le/firebase/firestore/model/value/ProtoValues.java 91.82% <100%> (ø) 106 <9> (+5) ⬆️

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 7ea5b37...aa7b3ec. Read the comment docs.

@schmidt-sebastian schmidt-sebastian changed the title Migrate ArrayTransforms to use Values Migrate transforms to use Values Feb 5, 2020
wilhuff
wilhuff approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java
@@ -66,6 +67,11 @@ public ObjectValue getData() {
return objectValue.get(path);
}

public @Nullable Value getFieldProto(FieldPath path) {
FieldValue fieldValue = getField(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this should go the other way: call ObjectValue.getFieldProto(path) here and then have getField above wrap the result of getFieldProto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ObjectValue.getFieldProto() does not yet exist (but it probably should).

That being said, this will go away with #1207, which will drop the FieldValue version altogether.

...in/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java Outdated
@@ -120,4 +121,12 @@ private long operandAsLong() {
+ operand.getClass().getCanonicalName());
}
}

private boolean isInteger(@Nullable Value value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These could be in ProtoValues along with isNumber and isArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...in/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java
return DoubleValue.valueOf(sum);
if (isInteger(baseValue) && operand instanceof IntegerValue) {
long sum = safeIncrement(baseValue.getIntegerValue(), operandAsLong());
return Value.newBuilder().setIntegerValue(sum).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Having Values in the production code would again benefit this code, making it less verbose to do this everywhere we do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Value conversion is now only available in UserDataReader, which is directly called from tests. The base branch for this PR removes the Values class.

@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/final to mrschmidt/rewritefieldvalue February 6, 2020 01:44
@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/rewritefieldvalue to mrschmidt/final February 6, 2020 01:46
wilhuff
wilhuff approved these changes Feb 6, 2020
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 6, 2020
@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/final to mrschmidt/rewritefieldvalue February 6, 2020 01:55
@schmidt-sebastian schmidt-sebastian merged commit 91b40a5 into mrschmidt/rewritefieldvalue Feb 6, 2020
@google-oss-bot
Copy link
Contributor

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests aa7b3ec link /test smoke-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/migratevalues branch February 7, 2020 00:22
@firebase firebase locked and limited conversation to collaborators Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wilhuff wilhuff wilhuff approved these changes

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
api: firestore cla: yes Override cla size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schmidt-sebastian @google-oss-bot @wilhuff @googlebot