Uh oh!
There was an error while loading. Please reload this page.
Firestore: Port @DocumentId to server SDK - #5743
Conversation
BenWhitehead
left a comment
There was a problem hiding this comment.
A few small questions, nothing major.
I assume this is being ported from the android SDK, is there a bread crumb we can to that implementation for a record of where this code is coming from?
| return data == null ? null : CustomClassMapper.convertToCustomClass(getData(), valueType); | ||
| return data == null | ||
| ? null | ||
| : CustomClassMapper.convertToCustomClass(getData(), valueType, docRef); |
There was a problem hiding this comment.
Is it valid to pass data in here, rather than calling getData() again?
| import java.util.Map; | ||
| /** A set of utilities for tests */ | ||
| public class TestUtil { |
There was a problem hiding this comment.
Can this be package local since it's only used for firestore tests?
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.9.8</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Duplicate dependency declaration, and formatting in consistent with the rest of the file.
| } | ||
| /** Holds information a deserialization operation needs to complete the job. */ | ||
| static class DeserializeContext { |
There was a problem hiding this comment.
Any reason this is package-private instead of private?
Codecov Report
@@ Coverage Diff @@## master #5743 +/- ##
============================================
+ Coverage 46.42% 47.17% +0.75% - Complexity 23633 25115 +1482
============================================
Files 2389 2389 Lines 254506 259831 +5325 Branches 28467 29420 +953 ============================================
+ Hits 118144 122582 +4438 - Misses 127547 128321 +774 - Partials 8815 8928 +113
Continue to review full report at Codecov.
|
ghost
left a comment
There was a problem hiding this comment.
The best doc i can find explaining this is https://docs.google.com/document/d/1seUfCzHSv7WXJgoJrtVMCh36HTco7lIrMHZlwCErs40/edit?usp=sharing
Requires corp account to view.
| <artifactId>jackson-databind</artifactId> | ||
| <version>2.9.8</version> | ||
| <scope>test</scope> | ||
| </dependency> |
| return data == null ? null : CustomClassMapper.convertToCustomClass(getData(), valueType); | ||
| return data == null | ||
| ? null | ||
| : CustomClassMapper.convertToCustomClass(getData(), valueType, docRef); |
| import java.util.Map; | ||
| /** A set of utilities for tests */ | ||
| public class TestUtil { |
| } | ||
| /** Holds information a deserialization operation needs to complete the job. */ | ||
| static class DeserializeContext { |
BenWhitehead
commented
Jul 17, 2019
Thanks for addressing my comments and adding the link to the doc @hui-wu. Can you run the code formatter again, that is one of the builds that are currently failing. |
| import java.util.Map; | ||
| /** A set of utilities for tests */ | ||
| class TestUtil { |
There was a problem hiding this comment.
There is already a TestUtil: https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java
Please move your helper to this class. Thanks!
schmidt-sebastian
left a comment
There was a problem hiding this comment.
LGTM, but please merge the two TestUtil classes.
Also add MapperTests.