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

Upgrade to Checkstyle 8.21 #113

Closed
snicoll opened this issue Jun 12, 2019 · 8 comments
Closed

Upgrade to Checkstyle 8.21 #113

snicoll opened this issue Jun 12, 2019 · 8 comments
Assignees
Milestone

Comments

@snicoll
Copy link
Contributor

snicoll commented Jun 12, 2019

This project uses the latest checkstyle release at that time (8.21). Trying to use 0.0.12 with that version on the initializr repository leads to:

Caused by: org.apache.maven.plugin.PluginExecutionException: Execution checkstyle-validation of goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check failed: An API incompatibility was encountered while executing org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check: java.lang.VerifyError: class io.spring.javaformat.checkstyle.SpringChecks overrides final method setTabWidth.(I)V
-----------------------------------------------------
realm =    plugin>org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0

This can be reproduced by upgrading the checkstyle version locally

@philwebb philwebb added this to the 0.0.x milestone Jun 12, 2019
@wilkinsona
Copy link
Contributor

It was added in 8.19.

@snicoll
Copy link
Contributor Author

snicoll commented Jun 13, 2019

Would it be possible to add an integration test in this project or something? The project currently build with that version and I'd expect it to fail.

@rajadilipkolli
Copy link
Contributor

Any temporary solution ?

@wilkinsona
Copy link
Contributor

You'll have to use Checkstyle 8.18 or earlier for now.

@philwebb philwebb changed the title SpringChecks overrides final method setTabWidth with Checkstyle 8.21 Upgrade to Checkstyle 8.21 Jun 24, 2019
@philwebb philwebb self-assigned this Jun 24, 2019
@philwebb philwebb modified the milestones: 0.0.x, 0.0.13 Jun 24, 2019
@wilkinsona
Copy link
Contributor

8.21 has changed how try-with-resources is treated and the formatter and Checkstyle now disagree. The formatter produces this:

try (InputStream foo = getInputStreamFor("foo"); InputStream bar = getInputStreamFor("bar")) {
   // …
}

Checkstyle reports an error due to multiple statements:

[ERROR] src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java:[66,64] (coding) OneStatementPerLine: Only one statement per line allowed.
[ERROR] src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java:[79,64] (coding) OneStatementPerLine: Only one statement per line allowed.
[ERROR] src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java:[92,64] (coding) OneStatementPerLine: Only one statement per line allowed.
[ERROR] src/test/java/org/springframework/boot/configurationmetadata/ConfigurationMetadataRepositoryJsonBuilderTests.java:[138,64] (coding) OneStatementPerLine: Only one statement per line allowed.

A change in Checkstyle is being discussed and a PR has been proposed. AIUI, the proposed change will mean that an error is no longer reported by default with a property to turn it on.

I'm not sure if we should:

  1. wait for a new Checkstyle release
  2. change the formatter to split the code onto multiple lines
  3. do something else

@philwebb
Copy link
Contributor

Changing the formatter is never easy so I'm tempted to fix it at checkstyle. Perhaps we can create our own local copy of that rule until the 8.22 is released.

@wilkinsona
Copy link
Contributor

There are only a handful of places in Boot that are affected. See parts of this commit. Another option could be to live with it and avoid having our own interim copy of the rule.

@philwebb
Copy link
Contributor

I think I prefer the multiple try blocks anyway, they seem a bit easier to read.

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

No branches or pull requests

4 participants