Skip to content

Conversation

@abhijeetshuklaoist
Copy link
Contributor

Signed-off-by: Abhijeet Shukla abhijeetshuklaoist@gmail.com

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #506 ☕️

@abhijeetshuklaoist abhijeetshuklaoist requested a review from a team as a code owner January 24, 2021 01:17
@abhijeetshuklaoist abhijeetshuklaoist requested a review from a team January 24, 2021 01:17
@abhijeetshuklaoist abhijeetshuklaoist requested a review from a team as a code owner January 24, 2021 01:17
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 24, 2021
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Jan 24, 2021
@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #508 (7585b9d) into master (1887932) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@ ## master #508 +/- ## ============================================ - Coverage 74.09% 74.06% -0.03%  + Complexity 1117 1116 -1  ============================================ Files 66 66 Lines 5883 5884 +1 Branches 723 724 +1 ============================================ - Hits 4359 4358 -1  Misses 1296 1296 - Partials 228 230 +2 
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/firestore/DocumentSnapshot.java 80.85% <0.00%> (-0.87%) 39.00% <0.00%> (ø%)
.../com/google/cloud/firestore/CustomClassMapper.java 76.68% <0.00%> (-0.17%) 110.00% <0.00%> (-1.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 1887932...7585b9d. Read the comment docs.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Another approach for this PR would be to simply drop the JSON parsing from the callsites. The JSON parser is only used in the Unit tests and it seems straightforward to replace its usage. Instead of:

 @Test public void primitiveDeserializeString() { StringBean bean = deserialize("{'value': 'foo'}", StringBean.class); assertEquals("foo", bean.value); ... 

We could just do:

 @Test public void primitiveDeserializeString() { StringBean bean = deserialize(map("value", "foo"), StringBean.class); // assertEquals("foo", bean.value); private static <T> T deserialize(Map<String, Object> value, Class<T> clazz) ... 

deserialize could also be:

 private static <T> T deserialize(Class<T> clazz, String key, Object value, Object ... moreKeysAndValues) ... 

Which would make the callsites even more concise.

@google-cla
Copy link

google-cla bot commented Jan 28, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jan 28, 2021
@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jan 28, 2021
@abhijeetshuklaoist abhijeetshuklaoist force-pushed the master branch 7 times, most recently from 02cce3c to f77c7da Compare January 28, 2021 17:55
@abhijeetshuklaoist
Copy link
Contributor Author

@googlebot I fixed it.

@elharo elharo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 3, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 3, 2021
@elharo elharo merged commit 7ada73d into googleapis:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.

4 participants