Skip to content
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

fix: add support for updating an individual field with pojo in all update method #136

Merged
merged 3 commits into from
Mar 27, 2020
Merged

fix: add support for updating an individual field with pojo in all update method #136

merged 3 commits into from
Mar 27, 2020

Conversation

suraj-qlogic
Copy link
Contributor

Fixes #126

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2020
@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2020
@athakor athakor requested a review from BenWhitehead March 18, 2020 14:15
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2020
@suraj-qlogic suraj-qlogic changed the title fix: support updating an individual field with pojo fix: add support for updating an individual field with pojo in all update method Mar 18, 2020
@codecov
Copy link

codecov bot commented Mar 18, 2020 •
edited
Loading

Codecov Report

Merging #136 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #136      +/-   ##
============================================
+ Coverage     71.56%   71.63%   +0.06%     
- Complexity      968      969       +1     
============================================
  Files            62       62              
  Lines          5202     5224      +22     
  Branches        579      579              
============================================
+ Hits           3723     3742      +19     
- Misses         1299     1302       +3     
  Partials        180      180
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/firestore/DocumentSnapshot.java 83.05% <100.00%> (+0.29%) 55.00 <0.00> (ø)
...n/java/com/google/cloud/firestore/Transaction.java 97.77% <0.00%> (ø) 11.00% <0.00%> (ø%)
...java/com/google/cloud/firestore/FirestoreImpl.java 82.35% <0.00%> (+0.28%) 24.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 50394e1...9bc0098. Read the comment docs.

BenWhitehead
BenWhitehead requested changes Mar 19, 2020
View reviewed changes
Copy link
Collaborator

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

Thanks @suraj-qlogic Can you add some tests to ensure that this fix is covering what is asked for in the original bug, and to ensure it doesn't accidentally get broken in the future.

@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
schmidt-sebastian
schmidt-sebastian reviewed Mar 25, 2020
View reviewed changes
google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java Outdated
@@ -191,6 +198,31 @@ public void setShortValue(@Nullable Short shortValue) {
}
}

public static class FooModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind re-using the SingleField example Pojo that exists in this class?

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

@schmidt-sebastian
Copy link
Contributor

@BenWhitehead This looks good to me, but we should try to replace the FooModel usage with SingleField before merging.

@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 27, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 27, 2020
schmidt-sebastian
schmidt-sebastian approved these changes Mar 27, 2020
View reviewed changes
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.

Thanks, this LGTM.

@BenWhitehead BenWhitehead merged commit 7d6c2c1 into googleapis:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schmidt-sebastian schmidt-sebastian schmidt-sebastian approved these changes

@BenWhitehead BenWhitehead BenWhitehead approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firestore: support updating an individual field with pojo as a value from all update methods
7 participants
@suraj-qlogic @schmidt-sebastian @BenWhitehead @googlebot @pmakani @yoshi-kokoro @athakor