Skip to content

#5075: add CheckStyle checking#5094

Closed
poikilotherm wants to merge 4 commits intoIQSS:developfrom
poikilotherm:5075-checkstyle
Closed

#5075: add CheckStyle checking#5094
poikilotherm wants to merge 4 commits intoIQSS:developfrom
poikilotherm:5075-checkstyle

Conversation

@poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Sep 25, 2018

As discussed during the community call, I gave it a shot 😉 . Please take this as a kind suggestion and discuss internally.

I took my default CheckStyle config first, which lead to about ~65k validations. The CheckStyle rules need to be tweaked to your "house style". If you want, I can help with this. Nearly everything is possible - this is a very mature tool.

If you want to try this: just clone my feature branch and activate the maven profile -Pvalidate. Enjoy! Or look into the Travis CI output for this.

Enhanced the Travis CI config too. If you want this in a separate issue/PR please comment, I'm willing to split things... 😄

(And please ignore the three other commits. This is - of course - based on work from #2940 and not yet merged #5071)

Related Issues

Pull Request Checklist

…mbined with JUnit 4.12 Vintage Engine.

This allows us to have both versions active in parallel and make the migration easier.

Fixed `IngestableDataCheckerTest.testTestSAVformat()` unit test, which was failing with 4.12.
This was not an engine fault but a mistake within the test case itself.
…{argLine}' in surefire plugin argLine option.
* Add CheckStyle configuration with most things commented out.
* Enabled Braces, FileTab and NewlineAtEnd checking
* Enhancing CI by adding stages to TravisCI configuration
* Do not run `mvn install` from TravisCI default, only install Maven dependencies (keep build log cleaner)
Copy link
Member

@michbarsinai michbarsinai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think we need to have a conversation on which checks to enable, but this is a good start.

@pdurbin
Copy link
Member

pdurbin commented Sep 27, 2018

@poikilotherm thank you very much for this pull request! I used it as the basis for pull request #5106 which is much smaller and what I feel is more of the "baby step" we need at this point. The build doesn't fail on violations. I simply offer some instructions on how to use Checkstyle to check your code if you feel like it. I'm closing this pull request in favor of that one.

@pdurbin pdurbin closed this Sep 27, 2018
@poikilotherm
Copy link
Contributor Author

Hi @pdurbin - sure. 👍 Better baby steps than nothing 😉 Whatever works for your team and how you can get peoples mind best changed to adapt to standard tooling and development routines.

Maybe you can reconsider my Travis CI configuration to get that mvn site thing up and running? The pipelining might be helpfull in different cases in the future 😉 .

@poikilotherm poikilotherm deleted the 5075-checkstyle branch November 29, 2018 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants