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

Introduce basic checkstyle configuration #98

Merged
merged 2 commits into from May 31, 2015
Merged

Introduce basic checkstyle configuration #98

merged 2 commits into from May 31, 2015

Conversation

gehel
Copy link
Contributor

@gehel gehel commented May 28, 2015

Style is now check during build at verify phase. Configuration is minimal and only checks for 2-space indentation at the moment. It can obviously be extended as required.

Build will fail if violations are found.

fixes #97

@gehel
Copy link
Contributor Author

gehel commented May 28, 2015

Let me know if you think this approach make sense for the project.

And let me know if you want me to rebase this on top of #96.

@chrisvest
Copy link
Owner

Looks like no rebasing is necessary.

Another rule I tend to follow is an 80 column line limit. Ideally that should only produce a warning, since there might be exceptions where longer lines are more pleasing to a human eye. I guess that could go into a separate PR if produces a lot of violations that should be checked over.

@gehel
Copy link
Contributor Author

gehel commented May 30, 2015

And I thought that everyone was using 120 or 130 as max line length those days ...

There are 30 violations on a line length of 80, 8 on a line length of 100 and none on a line length of 120.

Checkstyle does not have a mechanism to assign different priorities to different rules. I could configure checkstyle to run twice, with 2 different configurations, but I don't really like that option (files would be parsed twice...). Or configure checkstyle to only warn on violations for all rules.

My preferred option would be to configure a line length of 100 as a hard limit, correct the few places where that limit is violated and turn off checkstyle for the few places where it make sense to break that limit via inline comments.

@gehel
Copy link
Contributor Author

gehel commented May 30, 2015

I pushed another commit with line length configuration. I use stop checking line length and start checking line length as comment to mark code where we want to ignore this rule. I did some minor formatting corrections to ensure the rule is respected where it make sense.

Let me know if you want another configuration, or if you want me to squash those commits before merge.

gehel added 2 commits May 30, 2015 11:31
Style is now check during build at `verify` phase. Configuration is minimal and only checks for 2-space indentation at the moment. It can obviously be extended as required.

Build will fail if violations are found.

fixes #97
Added a check on line length, with a max line length of 100 chars.

Some code has been corrected to match this new rule. Specific checkstyle comments have been added to bypass this rule where it make sense.
@chrisvest
Copy link
Owner

This looks good. I think selectively turning line length checking off is a better solution than making them warnings. Makes it a conscious decision.

chrisvest added a commit that referenced this pull request May 31, 2015
Introduce basic checkstyle configuration
@chrisvest chrisvest merged commit 8406008 into chrisvest:master May 31, 2015
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.

Check style during build
2 participants