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.
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
Use google-java-format in spotless #1934
Conversation
Google-java-format is an open source formatter [1]. It automatically formats source code based on the Google Java Style. The Mockito source code to a very large extent already adheres to this style guide. While this PR in itself is large, many of the changes are related to string formatting and nested method calls. Most notably, google-java-format is an improvement over the current formatting strategy in that: 1. It handles comment formatting (e.g. spacing between comments) 2. It handles nested method calls. You can see the difference in our usage of the ByteBuddy API, which is now more consistent 3. It enforces the max-line length. It essentially automates all of the styling rules we list in https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment As such, for new contributors it should be a lot easier (and less scary) to contribute to Mockito, as they no longer have to be concerned about formatting. Hopefully, this once again lowers the bar for external contributors who want to help the project, but would otherwise feel overwhelmed by the rules we have to adhere to. (If we wouldn't have these rules, it would be a lot harder for us to maintain a consistent and maintainable codebase). The only interesting changes in this PR are those in `build.gradle`. All other changes were auto-generated by running `./gradlew spotlessApply`. Note that I disabled the formatting of Javadoc, as I think we should keep formatting that ourselves. We normally put a lot of time and effort in our Javadoc and changing that all at once seems like the wrong decision at this point in time. [1]: https://github.com/google/google-java-format
Ah I see GJF requires Java 11 to run, but we run on older versions of Java. We could configure Travis to only run the check on Java 11, since our formatting is not Java-version specific. |
@mockitoguy @raphw @bric3 Do you have any objections/thoughts on this PR? If you don't have any, I can polish the PR and make sure Travis happy and all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Make Travis happy and ship it!
I absolutely agree with the motivation behind this PR.
THANK YOU!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think these chages are great in general, there's part of the code where the auto format is huritng readability I think. Is there a way to tweak a bit for these cases.
I've highlighted a few issues, but probably missed some.
withSettings() | ||
.spiedInstance(instance) | ||
.defaultAnswer(Mockito.CALLS_REAL_METHODS) | ||
.name(field.getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above was easier to read imho.
Object injected = | ||
mockCandidateFilter | ||
.filterCandidate( | ||
mocks, candidateField, orderedCandidateInjecteeFields, injectee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to vertically allign parameters, instead. This change is weird to read.
final Collection<Object> mocks, | ||
final Field candidateFieldToBeInjected, | ||
final List<Field> allRemainingCandidateFields, | ||
final Object injectee) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the above was a tad better before.
this(new DefaultMockitoPlugins(), new PluginInitializer(pluginSwitch, null, new DefaultMockitoPlugins())); | ||
this( | ||
new DefaultMockitoPlugins(), | ||
new PluginInitializer(pluginSwitch, null, new DefaultMockitoPlugins())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to fix this annoying new line there ?
Thanks @mockitoguy and @bric3 for the review. I will relay the feedback about the formatting output to the google-java-format team and get them take a look. Since on the overall this change is a plus, I think we can merge without the fixes, but I will make sure they will be followed up on. I agree with your comments, so we can probably make a good case for fixing 😄 I will address the Travis changes in a separate PR, as that requires some cleanup there as well. Then I will rebase this PR and regenerate the formatting changes. Thanks for the quick response after the ping, greatly appreciated! |
This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of #1934 which requires spotless to run on JDK11 minimum.
👍 |
This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of #1934 which requires spotless to run on JDK11 minimum.
Codecov Report
@@ Coverage Diff @@
## release/3.x #1934 +/- ##
=================================================
- Coverage 86.21% 85.76% -0.45%
Complexity 2542 2542
=================================================
Files 318 318
Lines 6738 7209 +471
Branches 838 861 +23
=================================================
+ Hits 5809 6183 +374
- Misses 715 810 +95
- Partials 214 216 +2
Continue to review full report at Codecov.
|
Most of the coverage changes are related to having more lines in exception messages that are untested. Functionally, we haven't lost coverage. https://codecov.io/gh/mockito/mockito/pull/1934/changes also doesn't list any file, which leads me to that conclusion. Therefore, I am landing this now. We can improve our code coverage in separate CLs (we are still at 85%, which is quite good) |
This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of mockito#1934 which requires spotless to run on JDK11 minimum.
Google-java-format is an open source formatter [1]. It automatically formats source code based on the Google Java Style. The Mockito source code to a very large extent already adheres to this style guide. While this PR in itself is large, many of the changes are related to string formatting and nested method calls. Most notably, google-java-format is an improvement over the current formatting strategy in that: 1. It handles comment formatting (e.g. spacing between comments) 2. It handles nested method calls. You can see the difference in our usage of the ByteBuddy API, which is now more consistent 3. It enforces the max-line length. It essentially automates all of the styling rules we list in https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment As such, for new contributors it should be a lot easier (and less scary) to contribute to Mockito, as they no longer have to be concerned about formatting. Hopefully, this once again lowers the bar for external contributors who want to help the project, but would otherwise feel overwhelmed by the rules we have to adhere to. (If we wouldn't have these rules, it would be a lot harder for us to maintain a consistent and maintainable codebase). The only interesting changes in this PR are those in `build.gradle`. All other changes were auto-generated by running `./gradlew spotlessApply`. Note that I disabled the formatting of Javadoc, as I think we should keep formatting that ourselves. We normally put a lot of time and effort in our Javadoc and changing that all at once seems like the wrong decision at this point in time. [1]: https://github.com/google/google-java-format
Google-java-format is an open source formatter 1. It automatically formats
source code based on the Google Java Style.
The Mockito source code to a very large extent already adheres to this style
guide. While this PR in itself is large, many of the changes are related to
string formatting and nested method calls. Most notably, google-java-format
is an improvement over the current formatting strategy in that:
our usage of the ByteBuddy API, which is now more consistent
It essentially automates all of the styling rules we list in
https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment
As such, for new contributors it should be a lot easier (and less scary)
to contribute to Mockito, as they no longer have to be concerned about
formatting. Hopefully, this once again lowers the bar for external contributors
who want to help the project, but would otherwise feel overwhelmed by
the rules we have to adhere to. (If we wouldn't have these rules, it would
be a lot harder for us to maintain a consistent and maintainable codebase).
The only interesting changes in this PR are those in
build.gradle
. Allother changes were auto-generated by running
./gradlew spotlessApply
.Note that I disabled the formatting of Javadoc, as I think we should keep formatting
that ourselves. We normally put a lot of time and effort in our Javadoc and changing
that all at once seems like the wrong decision at this point in time.