Add missing checkstyle throws indentation setting #1538

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

scop commented Feb 14, 2014

Without this I see quite a few "method/ctor def throws at indentation level X not at correct indentation, Y" errors in Eclipse, seemingly for all throws statements that are placed on a new line.

Can one of the admins verify this patch by saying "ok to test"?

Owner

jbertram commented Feb 14, 2014

ok to test

Owner

clebertsuconic commented Feb 14, 2014

it's failing with .... Property 'throwsIndent' in module Indentation does not exist, please check the documentation -

did you change your version on your pom or anything?

Owner

jbertram commented Feb 14, 2014

Couple of things:

  1. When I merge this change locally and run 'mvn checkstyle:check' I get this error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.10:check (default-cli) on project hornetq-pom: Failed during checkstyle configuration: cannot initialize module TreeWalker - Property 'throwsIndent' in module Indentation does not exist, please check the documentation -> [Help 1]

  1. If there is a problem with the code which a new Checkstyle rule would detect then I would expect code changes to be committed along with the Checkstyle change so that the build won't fail.

  2. All the core developers of which I am aware use IntelliJIDEA so we don't see these errors.

In the end, you'll need to find a different way to address the errors you're seeing in Eclipse.

jbertram closed this Feb 14, 2014

Contributor

scop commented Feb 15, 2014

I'm fairly certain you'll start to see these errors yourself when your setup gets updated so that it pulls in checkstyle >= 5.7: http://checkstyle.sourceforge.net/releasenotes.html ("added support for a separate throws indentation configuration"). Apparently my Eclipse setup is running with checkstyle 5.7 already for some reason.

There's another way to appease both the old and new checkstyle; however in my opinion it is a somewhat dubious and verbose fix for the problem but not entirely inconsistent with the rest of the HornetQ code base, see PR #1542 for that.

Owner

clebertsuconic commented Feb 15, 2014

I have no issues on merging this as long as the pom is fixed.. do you have an idea why mvn install doesn't work here? It's complaining about the new property you added not being supported by checkstyle

scop referenced this pull request in checkstyle/checkstyle Feb 16, 2014

Closed

Default case and throws indent to basic offset #108

Contributor

scop commented Feb 16, 2014

Yes, it's because the checkstyle pulled in by maven-checkstyle-plugin is 5.6 while I have checkstyle 5.7 in Eclipse, and the property I added was introduced in 5.7. I don't know maven well enough to tell if it's possible to tell maven-checkstyle-plugin to use checkstyle 5.7; its pom refers to version 5.6 (still in the latest version 2.11 at the moment).

BTW I'm trying to convince checkstyle people to default all indentation settings to basicOffset unless specifically configured otherwise, see checkstyle/checkstyle#108

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