Skip to content

Drop remaining primitive FieldValue types #1207

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

Drop remaining primitive FieldValue types #1207

merged 26 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 6, 2020
@codecov
Copy link

codecov bot commented Feb 6, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (mrschmidt/rewritefieldvalue@3f2efb4). Click here to learn what that means.
The diff coverage is 100%.

Flag Coverage Δ Complexity Δ
#FirebaseFirestore 61.95% <100%> (?) 2230 <1> (?)
#FirebaseFirestore_Ktx 41.17% <ø> (?) 0 <ø> (?)
Impacted Files Coverage Δ Complexity Δ
...le/firebase/firestore/model/value/ProtoValues.java 90.9% <ø> (ø) 116 <0> (?)
...gle/firebase/firestore/model/value/FieldValue.java 100% <100%> (ø) 8 <1> (?)

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 3f2efb4...2ce8a22. Read the comment docs.

@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/droparray to master February 6, 2020 00:13
@schmidt-sebastian schmidt-sebastian changed the base branch from master to mrschmidt/droparray February 6, 2020 00:14
@schmidt-sebastian schmidt-sebastian mentioned this pull request Feb 6, 2020
Merged
@schmidt-sebastian
Copy link
Contributor Author

/test check-changed

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/value/ProtoValues.java Outdated
@@ -41,34 +41,47 @@
public static final Value NULL_VALUE =
Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build();

/** The order of types in Firestore; this order is defined by the backend. */
private static final int TYPE_ORDER_NULL = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

public methods that return private values make little sense. typeOrder should either be private or these constants should be public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made them public.

firebase-firestore/src/test/java/com/google/firebase/firestore/UserDataWriterTest.java Outdated
@@ -291,4 +287,8 @@ public void testRejectsJavaArrays() {
private Object convertValue(FieldValue value) {
return writer.convertValue(value.getProto());
}

private void assertValueType(FieldValue value, Value.ValueTypeCase booleanValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JUnit tends to put the expected value as the first argument. Consider reversing these to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this. Note that I have a similar helper for canonical IDs, which I kept the same as the callsites look much cleaner in reverse argument order.

firebase-firestore/src/test/java/com/google/firebase/firestore/UserDataWriterTest.java
"b", fromMap("c", StringValue.valueOf("foo")), "d", BooleanValue.valueOf(true)));
assertEquals(expected, actual);
ObjectValue.Builder expected = ObjectValue.newBuilder();
expected.set(field("a.b.c"), valueOf("foo"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: You could chain these (ObjectValue expected = newBuilder.set.set.build())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt cleaner to spread these assignments over multiple lines. Kept as is.

@wilhuff wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 6, 2020
@schmidt-sebastian schmidt-sebastian changed the base branch from mrschmidt/droparray to mrschmidt/rewritefieldvalue February 6, 2020 19:11
@schmidt-sebastian schmidt-sebastian merged commit 3c6d8d1 into mrschmidt/rewritefieldvalue Feb 6, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/droprest branch February 7, 2020 00:22
@firebase firebase locked and limited conversation to collaborators Mar 8, 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 @wilhuff @googlebot @google-oss-bot