[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:
Blocks
blocks OKAPI-809 Google Code Style plugin and conformance Closed
blocks CIRC-619 Use check style to validate google co... Blocked
Relates
relates to FOLIO-1667 Agree on Java Import Ordering (and ex... Closed
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
RMB's existing checkstyle configuration: https://github.com/folio-org/raml-module-builder/blob/master/codestyles.xml , https://github.com/folio-org/raml-module-builder/blob/master/codestyles-suppressions.xml

Checkstyle supports both styles:
https://checkstyle.sourceforge.io/sun_style.html
https://checkstyle.sourceforge.io/google_style.html

We have a SonarCloud Java configuration: https://sonarcloud.io/organizations/folio-org/rules?languages=java
Some code style convention rules are enabled, many are disabled but we can enable them. IDEs support Sonar, and most back-end modules have a SonarCloud report on each pull request.



 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 CIRC-619 Blocked ).

In that branch, it includes some customisations to the style:

  • Do not require comment for a public method
  • Use Eclipse import order rule
  • Use 2 space indentation (to align with the current standard)

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 ]
  • I actually prefer the line wrapping indentation of 4. Then again, I wasn't aware that line wrapping indentation was 2 for FOLIO. I believe I always used 4 in my code for line wrapping. Yup, just confirmed I used a line wrap indentation of 4 with some files I created in mod-circulation.
  • I could go either way with the import order. I actually don't mind alphabetical order (google) vs grouped alphabetical order (eclipse).
  • The public method comments only apply to methods that are over a certain amount of lines (maybe 2?), but I can see that being tedious to fill in existing missing methods even though I think some doc may be helpful to consumers of those public methods.

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?
FOLIO's SonarQube/SonarCloud/SonarLint documentation: https://dev.folio.org/guides/code-analysis/#sonarqube
FOLIO's current Sonar configuration: https://sonarcloud.io/organizations/folio-org/quality_profiles/show?language=java&name=FOLIO+Java+Way
IDE (IntelliJ, Eclipse, Visual Studio Code) Sonar plugin: https://www.sonarlint.org/

Comment by Marc Johnson [ 23/Mar/20 ]

Julian Ladisch

Can someone explain whether this issue is also about switching from Sonarcloud and SonarLint to Checkstyle?

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:
"we cannot even maintain Check in current state , too much bugs in it, so making it even more complicated it not doable. We can come back to this idea after we fix all existing problems in this Check." https://github.com/checkstyle/checkstyle/issues/3342#issuecomment-502492718

Java Lambdas are supported by the autoindent feature of IDEs like IntelliJ and Eclipse, but not by checkstyle.
Checkstyle incorrectly says that the second lambda of this code example from https://vertx.io/docs/vertx-core/java/#blocking_code needs more indentation:

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:
https://github.com/checkstyle/checkstyle/issues/3182
https://github.com/checkstyle/checkstyle/issues/3342
https://github.com/checkstyle/checkstyle/issues/3415
https://github.com/checkstyle/checkstyle/issues/4006
https://github.com/checkstyle/checkstyle/issues/4638
https://github.com/checkstyle/checkstyle/issues/5969
https://github.com/checkstyle/checkstyle/issues/6106
https://github.com/checkstyle/checkstyle/issues/7675
https://github.com/checkstyle/checkstyle/pull/7899

Sonar has this Java indentation rule: https://sonarcloud.io/organizations/folio-org/rules?open=java%3AS1120&rule_key=java%3AS1120
It is disabled but it can easily been enabled.

Sonar currently does not support enforcing import order for the reasons explained in
https://community.sonarsource.com/t/java-check-orderings-of-imports/17302
We may contribute code to sonar to add a default Eclipse import order check.
We may use checkstyle for only the import order check and use sonar otherwise.
The checkstyle configuration to match default Eclipse formatter configuration for Eclipse Mars onwards is on
https://checkstyle.sourceforge.io/config_imports.html#ImportOrder_Examples

Sonar has 48 convention and style rules for Java we can pick from:
https://sonarcloud.io/organizations/folio-org/rules?languages=java&tags=convention%2Cstyle
Which expression and line formatting rules are missing?

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
This is checked by sonar: https://sonarcloud.io/organizations/folio-org/rules?open=java%3AS131&rule_key=java%3AS131
The checkstyle rule does not check for enum completeness, is more restrictive than Google code style, doesn't detect missing enum values and should therefore been disabled: https://checkstyle.sourceforge.io/config_coding.html#MissingSwitchDefault

Comment by Marc Johnson [ 24/Mar/20 ]

Julian Ladisch

Thank you for researching this.

The current maintainer of checkstyle romani says about checkstyle's indentation check:
"we cannot even maintain Check in current state , too much bugs in it, so making it even more complicated it not doable. We can come back to this idea after we fix all existing problems in this Check."

I didn't know about this, I haven't tried to use checkstyle much.

Java Lambdas are supported by the autoindent feature of IDEs like IntelliJ and Eclipse, but not by checkstyle.

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.

Sonar has this Java indentation rule. It is disabled but it can easily been enabled.

Do you know why it is turned off currently?

Sonar currently does not support enforcing import order

That's a shame

We may use checkstyle for only the import order check and use sonar otherwise.
The checkstyle configuration to match default Eclipse formatter configuration for Eclipse Mars onwards is on

This would seem to act against the goal of not introducing another tool :-/

Which expression and line formatting rules are missing?

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.

Generated at Thu Feb 08 23:18:25 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.