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.

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

Java code formatter #863

Closed
tuchida opened this issue Apr 13, 2021 · 7 comments
Closed

Java code formatter #863

tuchida opened this issue Apr 13, 2021 · 7 comments

Comments

@tuchida
Copy link
Contributor

tuchida commented Apr 13, 2021

I would like a code formatter to unify coding styles.

  • Use gradle to check and format (for example: ./gradlew format)
  • (Optional) Checking with CI
  • (Optional) Integration with editors

Is this a good idea to use?
https://github.com/sherter/google-java-format-gradle-plugin

@tuchida tuchida mentioned this issue Apr 13, 2021
@tuchida
Copy link
Contributor Author

tuchida commented Apr 13, 2021

They are still using Checkstyle to check the coding style.
https://github.com/mozilla/rhino/blob/9ebcb56b7461b01d809c40a281db3c75f43f20d7/checkstyle.xml

./gradlew check

However, Checkstyle did not seem to have an autofix feature.

https://github.com/sherter/google-java-format-gradle-plugin

I tried to use this plugin, but I couldn't figure out where the syntax error was.

I tried this.
https://github.com/diffplug/spotless/tree/main/plugin-gradle#google-java-format

diff --git a/build.gradle b/build.gradle
index 249b1a49..019af96d 100644
--- a/build.gradle
+++ b/build.gradle
@@ -8,6 +8,7 @@ plugins {
     id 'distribution'
     id 'checkstyle'
     id 'com.github.spotbugs' version "4.6.2"
+    id "com.diffplug.spotless" version "5.12.1"
 }
 
 tasks.withType(JavaCompile) {
@@ -25,6 +26,12 @@ repositories {
     mavenCentral()
 }
 
+spotless {
+    java {
+        googleJavaFormat('1.7').aosp()
+    }
+}
+
 sourceSets {
     main {
         java {
# check
$ ./gradlew spotlessCheck

# autofix
$ ./gradlew spotlessApply

There was a very large amount of revision.

@gbrail
Copy link
Collaborator

gbrail commented Apr 13, 2021

As you figured out, checkstyle is already in the build but except for some very obvious rules it's mostly turned off. It's no issue at all to adjust the checkstyle rules to enforce more things, or to replace it with another formatter.

The problem is that adopting any formatter would introduce a multi-thousand-line set of changes to the codebase. Everyone who has an outstanding PR, or a branch with a bit of experimental things, or a fork of Rhino, would have a messy merge on their hands.

I can think of a few ways to make this less painful:

  1. Introduce rules a few at a time using checkstyle, so that there are only a few hundred thousand changes at once rather than tens of thousands
  2. Agree on some sort of "moratorium" time after which everyone with an outstanding PR is going to be expected to merge all the formatting changes and resolve all the conflicts.

But I have held off on doing anything like this because of the aforementioned merge problems.

If there were some code formatter out there that we could annotate with "already fixed" or something so that it'd only complain about certain files, then perhaps that would be another way.

@rbri
Copy link
Collaborator

rbri commented Apr 13, 2021 via email

@tuchida
Copy link
Contributor Author

tuchida commented Apr 15, 2021

I can think of a few ways to make this less painful:

If you have the auto-formatting feature, you can simply run the command to remove the error.
If it were only a check feature, it would take an effort to correct even a minor rule change.

@gbrail
Copy link
Collaborator

gbrail commented Apr 15, 2021

This may not be as bad as I had feared, since spotless has the "ratchet" feature. Check out #866 and if you like it we'll do it!

@rbri
Copy link
Collaborator

rbri commented Apr 15, 2021

Fine for me +1

@tuchida
Copy link
Contributor Author

tuchida commented Apr 15, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants