-
Notifications
You must be signed in to change notification settings - Fork 142
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
JCLOUDS-1497: Fix checkstyle-suppressions for jcloud-labs #27
Conversation
Closing and re-opening to trigger the build again |
@@ -48,7 +48,7 @@ | |||
</module> | |||
<module name="NoWhitespaceBefore"/> | |||
<module name="RedundantImport"/> | |||
<module name="RedundantModifier"/> | |||
<!-- <module name="RedundantModifier"/> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR LGTM, but why are you removing this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some static imports (search for "import static") in the code. I am doubting it is worth the effort to fix it. Wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why this check was not failing before. Anyway, I agree there is little value in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should find redundant modifiers like public
in interfaces, not static imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I ran a local build and only in core
there were more than 40 violations. I fixed them, then found the same in another project... So I agree the value this check provides is close to zero and I think removing it is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I run the report again, and yeah sorry you are right: The violations where way down the file not on top where the "import static" was used.
This is the jclouds part of fixing checkstyle. jclouds-labs has a couple of violations as well