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

feat: Add toString methods to classes comprising WriteBatch #1281

Merged
merged 7 commits into from
Jun 6, 2023
Merged

feat: Add toString methods to classes comprising WriteBatch #1281

merged 7 commits into from
Jun 6, 2023

Conversation

eranl
Copy link
Contributor

@eranl eranl commented May 1, 2023

For development, it would be very helpful to have toString methods on classes comprising WriteBatch. It would make it easier to see the data in the debugger and log it in dry-run mode.

Fixes #1280 ☕️

@eranl eranl requested a review from a team as a code owner May 1, 2023 19:07
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: firestore Issues related to the googleapis/java-firestore API. labels May 1, 2023
@tom-andersen
Copy link
Contributor

Your change is very straight forward. I am inclined to approve, but would like a second from the team.

@tom-andersen tom-andersen self-assigned this May 1, 2023
@eranl eranl requested a review from tom-andersen May 2, 2023 00:50
@tom-andersen
Copy link
Contributor

https://github.com/googleapis/java-firestore/blob/main/CONTRIBUTING.md

Code in this repo is formatted with google-java-format. To run formatting on your project, you can run:

mvn com.coveo:fmt-maven-plugin:format

tom-andersen
tom-andersen requested changes May 3, 2023
View reviewed changes
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

Please add a unit test for each toString method.

You can create a new file in src/test/java/com/google/cloud/firestore for all toString() unit tests.

This is a slightly higher standard, since we haven't written unit tests for other toString() methods, so I hope you don't mind helping us improve our code quality. This also helps reviewers by providing an example of output.

I enabled CI on this PR. Whatever it complains about (ie code formatting), will need to be fixed before we can merge.

@tom-andersen tom-andersen added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 3, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 3, 2023
@eranl
feat: Add toString methods to classes comprising WriteBatch
885046d 
take3:
- fix formatting
- add unit tests
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels May 6, 2023
@eranl
Copy link
Contributor Author

eranl commented May 6, 2023

Just committed take 3.
Sorry about forgetting to format the code. Done now.
I added a few unit tests. I hope that's what you had in mind. There's risk of high maintenance cost for such tests, so I'm not sure they're worth having.

@eranl eranl requested a review from tom-andersen May 6, 2023 00:59
@tom-andersen tom-andersen added the kokoro:run Add this label to force Kokoro to re-run the tests. label May 8, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label May 8, 2023
dconeybe
dconeybe requested changes May 8, 2023
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

First of all, thank you for taking the time to open this PR! I'm sure other customers will enjoy the benefits of this work.

@tom-andersen asked me to take a look at your PR so that we have "two sets of eyes" on it. I've left four comments that hopefully will not be too difficult to resolve.

google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentSnapshot.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/ToStringTest.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/ToStringTest.java Outdated Show resolved Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/ToStringTest.java Outdated Show resolved Hide resolved
@eranl
Copy link
Contributor Author

eranl commented Jun 2, 2023

Just pushed take 4, which addresses @dconeybe's review comments, and limits verification to the toString implementation of the classes under test, as opposed to those of their fields.

@eranl eranl requested a review from dconeybe June 2, 2023 14:07
@tom-andersen
Copy link
Contributor

Thanks @eranl.

Denver has been out of office, so there might be a delay before he has a chance to review.

dconeybe
dconeybe requested changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Thank you for applying the suggestions. It looks great! I just have 1 very minor request, then I'll happily approve.

google-cloud-firestore/src/test/java/com/google/cloud/firestore/ToStringTest.java Outdated Show resolved Hide resolved
@eranl
Copy link
Contributor Author

eranl commented Jun 2, 2023

Pushed take 5, addressing final comment

@eranl eranl requested a review from dconeybe June 2, 2023 22:50
dconeybe
dconeybe approved these changes Jun 3, 2023
View reviewed changes
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Thank you for bearing with us through the review process and for contributing to Firestore!

@eranl
Copy link
Contributor Author

eranl commented Jun 3, 2023

Thank you @dconeybe for your thoughtful and timely review!
@tom-andersen, it looks like your approval is now required.

@tom-andersen tom-andersen added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Jun 6, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 6, 2023
@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 6, 2023
@tom-andersen tom-andersen added owlbot:ignore instruct owl-bot to ignore a PR owlbot:run Add this label to trigger the Owlbot post processor. and removed owlbot:ignore instruct owl-bot to ignore a PR labels Jun 6, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 6, 2023
@gcf-owl-bot gcf-owl-bot bot requested a review from a team as a code owner June 6, 2023 12:34
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 6, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 6, 2023
@tom-andersen tom-andersen merged commit 9df8028 into googleapis:main Jun 6, 2023
19 of 20 checks passed
@release-please release-please bot mentioned this pull request Jun 6, 2023
Merged
cherylEnkidu pushed a commit that referenced this pull request Dec 11, 2023
@eranl @gcf-owl-bot @cherylEnkidu
feat: Add toString methods to classes comprising WriteBatch (#1281)
6861ca0 
* feat: Add toString methods to classes comprising WriteBatch

* feat: Add toString methods to classes comprising WriteBatch
take2: use dynamic class names

* feat: Add toString methods to classes comprising WriteBatch
take3:
- fix formatting
- add unit tests

* feat: Add toString methods to classes comprising WriteBatch
take4: address review comments

* feat: Add toString methods to classes comprising WriteBatch
take5: address review comment

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
@release-please release-please bot mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-andersen tom-andersen tom-andersen approved these changes

@dconeybe dconeybe dconeybe approved these changes

Assignees

@eranl eranl

@tom-andersen tom-andersen

@dconeybe dconeybe

Labels
api: firestore Issues related to the googleapis/java-firestore API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add toString methods to classes comprising WriteBatch
4 participants
@eranl @tom-andersen @dconeybe @yoshi-kokoro