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

Add missing checkstyle throws indentation setting #1538

Closed
wants to merge 1 commit into from
Closed

Add missing checkstyle throws indentation setting #1538

wants to merge 1 commit into from

Conversation

scop
Copy link
Contributor

@scop 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.

@HornetQBot
Copy link

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

@jbertram
Copy link
Member

ok to test

@clebertsuconic
Copy link
Member

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?

@jbertram
Copy link
Member

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.

@scop
Copy link
Contributor Author

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.

@clebertsuconic
Copy link
Member

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
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
4 participants