[FOLIO-2127] Decision: Switch to Google Java style guide? Created: 28/Jun/19 Updated: 25/Jan/23 Resolved: 25/Jan/23 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Task | Priority: | TBD |
| Reporter: | Marc Johnson | Assignee: | Unassigned |
| Resolution: | Won't Do | Votes: | 0 |
| Labels: | potential-decision, tech-debt | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||||||
| Sprint: | |||||||||||||||||||||
| Development Team: | Core: Platform | ||||||||||||||||||||
| Description |
|
The Google Java style guide is modern and maintained and is supported out of the box by check style, which could make it easier to be consistent. This issue is about making the decision whether we want to make the change from sun coding style to google coding style. FOLIO's current coding style: https://dev.folio.org/guidelines/contributing/#coding-style Checkstyle supports both styles: We have a SonarCloud Java configuration: https://sonarcloud.io/organizations/folio-org/rules?languages=java |
| Comments |
| Comment by Matt Reno [ 28/Jun/19 ] |
|
Here are some pros/cons for this. The Google style guide is similar to our existing Java style. One way to enforce this is by using the maven-checkstyle-plugin in the pom file. A simple example is:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.0.0</version>
<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<configLocation>google_checks.xml</configLocation>
<linkXRef>false</linkXRef>
<violationSeverity>warning</violationSeverity>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<sourceDirectories>
<sourceDirectory>${project.build.sourceDirectory}</sourceDirectory>
<sourceDirectory>${project.build.testSourceDirectory}</sourceDirectory>
</sourceDirectories>
</configuration>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.18</version>
</dependency>
</dependencies>
</plugin>
In the above configuration, the sourceDirectories config is to ignore the RMB generated code, since it does not conform. If RMB could generate compliant code, this could be removed. This will use a built in google_checks.xml file, but that may be too restrictive for modules and exceptions may need to be made. For example, Google requires at most 1 capital letter in a row in method names, variables, etc., and in the edge-sip2 module we had cases where we needed multiple uppercase letters in a row. To overcome this, we provided a custom google_checks.xml stored in the project as "checkstyle.xml" that has a white list of allowed abbreviations. In the pom, we reference "checkstyle.xml" in the configLocation. The obvious downside here is that if improvements are made to the original google_checks.xml file, then they'd need to be discovered somehow and then merged in manually. Checkstyle does not support inheritance or custom overrides at this point, just suppressions (which can be too extreme) or elaborate pom file hacks to get around this. Eclipse has a plugin for checkstyle that works reasonably well. It can use a custom checkstyle.xml file as well. Other IDEs probably have reasonable support for checkstyle. The key here is that the build will fail if there is a violation. In existing projects, we may not want to fail on a violation since there may be thousands that need to be corrected. But the ultimate goal should be to fail the build if there is a violation. Most common violations will likely be import ordering, exceeding max line length, incorrect indentation, acronyms in method/variable/class names, and missing javadoc for public methods. Of course, we could come up with our own style or use the Sun Java style, but each of those has drawbacks. Coming up with our own style takes time and will be frustrating to come to consensus. The Sun Java style has a lot of restrictions that we don't comply with, far more than the google style, which would lead to more violations that need to be fixed. |
| Comment by Marc Johnson [ 05/Mar/20 ] |
|
Jakub Skoczen Craig McNally Matt Reno As I understand it, the tech-leads agreed to trial adoption of the Google Java Code Style within mod-circulation. Is that a sensible recollection? |
| Comment by Matt Reno [ 05/Mar/20 ] |
|
Marc Johnson I believe that is correct. |
| Comment by Craig McNally [ 09/Mar/20 ] |
|
I don't remember that conversation but it seems fine to me. |
| Comment by Marc Johnson [ 20/Mar/20 ] |
|
Bohdan Suprun has prepared a branch for trying out the checkstyle Google code style (see
In that branch, it includes some customisations to the style:
As Matt Reno states above, there are trade offs to us defining our own style. Do we want to make these amendments to the style, and maintain a FOLIO custom style? |
| Comment by Matt Reno [ 20/Mar/20 ] |
My 2 cents, for what it's worth, would be we stick with vanilla style so most projects can just use the latest style file provided by the maven plugin (no project local checkstyle file). If there is a reason that we need to tweak the file, we can do that on a project by project basis. |
| Comment by Julian Ladisch [ 23/Mar/20 ] |
|
Can someone explain whether this issue is also about switching from Sonarcloud and SonarLint to Checkstyle? |
| Comment by Marc Johnson [ 23/Mar/20 ] |
That is an excellent question, thank you for asking. I run the Sonar plugin in IntelliJ using the SonarQube configuration, and I find it useful for not having to wait for CI. I think CheckStyle was suggested during a retrospective following concerns around the amount of time during pull requests spent advising on formatting issues. As I understand it, the current Sonar configuration does not check indentation, import ordering, expression and line formatting etc. If Sonar can provide static analysis for formatting, import order etc and that can run locally in a maven build, then that is great and preferable to introducing new tools. |
| Comment by Julian Ladisch [ 23/Mar/20 ] |
|
The current maintainer of checkstyle romani says about checkstyle's indentation check: Java Lambdas are supported by the autoindent feature of IDEs like IntelliJ and Eclipse, but not by checkstyle.
vertx.executeBlocking(promise -> {
// Call some blocking API that takes a significant amount of time to return
String result = someAPI.blockingMethod("hello");
promise.complete(result);
}, res -> {
System.out.println("The result is: " + res.result());
});
List of indentation issues related to lambdas: Sonar has this Java indentation rule: https://sonarcloud.io/organizations/folio-org/rules?open=java%3AS1120&rule_key=java%3AS1120 Sonar currently does not support enforcing import order for the reasons explained in Sonar has 48 convention and style rules for Java we can pick from: To run Sonar locally in a maven build use https://docs.sonarqube.org/latest/analysis/scan/sonarscanner-for-maven/ |
| Comment by Julian Ladisch [ 23/Mar/20 ] |
|
Google code style suggests to use an enum without default when listing all enum values: https://google.github.io/styleguide/javaguide.html#s4.8.4.3-switch-default |
| Comment by Marc Johnson [ 24/Mar/20 ] |
|
Thank you for researching this.
I didn't know about this, I haven't tried to use checkstyle much.
Alas, a lot of the formatting issues I asked to be changed during pull requests are usually due to an IDE doing it automatically. I'm happy for folks to use IDE's to automatically format code. I don't think we should rely on it, and would prefer a tool that can be used in the automated build process.
Do you know why it is turned off currently?
That's a shame
This would seem to act against the goal of not introducing another tool :-/
I'm mostly interested in function parameters spanning more than one line, that seems to be incorrectly formatted a lot. |
| Comment by Adam Dickmeiss [ 25/Mar/20 ] |
|
Checkstyle looks like very maintained when you look at the commits. There are many checks that Sonar Qube did not spot.. Perhaps that was just the way we configured Sonar Qube. We have only found a minor issue with switch-default-handling.. And indentation of multiple lambdas . Other than that, it seems to work fine. |