Skip to content

Add Checkstyle framework#3551

Merged
b-slim merged 5 commits intoapache:masterfrom
leventov:checkstyle
Oct 13, 2016
Merged

Add Checkstyle framework#3551
b-slim merged 5 commits intoapache:masterfrom
leventov:checkstyle

Conversation

@leventov
Copy link
Copy Markdown
Member

@leventov leventov commented Oct 6, 2016

Fixes #3533.

This PR just adds the Checkstyle framework without enforcing any rules yet; they are supposed to be added one-by-one in different PRs.

Checkstyle's XML configs are placed in a directory named codestyle because there is an intention to move druid_intellij_formatting.xml, eclipse_formatting.xml and eclipse.importorder there eventually.

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Oct 6, 2016

is this using the druid template to check stuff ?

@leventov
Copy link
Copy Markdown
Member Author

leventov commented Oct 6, 2016

@b-slim it doesn't check anything yet. Eventually we should add checks to enforce druid's rules. Two examples are commented in checkstyle.xml.

@cheddar
Copy link
Copy Markdown
Contributor

cheddar commented Oct 7, 2016

Is this PR a WIP? If it's not a WIP, I politely request that we make it a WIP given that it doesn't actually enforce anything yet.

@leventov
Copy link
Copy Markdown
Member Author

leventov commented Oct 7, 2016

@cheddar added some checks. Don't want to make this PR too big though. It's impossible to make checkstyle to enforce the whole Druid's code style in one PR.

@cheddar
Copy link
Copy Markdown
Contributor

cheddar commented Oct 8, 2016

👍

Totally understand that it's not really possible to do everything. But also, introducing just boilerplate that doesn't do anything is equivalent to just adding cruft. Thanks for making this check something and fixing all the things.

Comment thread codestyle/checkstyle-suppressions.xml Outdated

<suppressions>
<!-- See http://checkstyle.sourceforge.net/config_filters.html#SuppressionFilter for examples -->
</suppressions> No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should it be a new line in the end of the file?

Comment thread codestyle/checkstyle.xml Outdated
<module name="RedundantImport"/>
<module name="NeedBraces"/>
</module>
</module> No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should it be a new line in the end of the file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@logarithm thanks. Made Checkstyle to check this, btw :)

@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Oct 13, 2016

👍

@b-slim b-slim merged commit 5dc9538 into apache:master Oct 13, 2016
@leventov leventov deleted the checkstyle branch October 13, 2016 22:24
fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016
* Add Checkstyle framework

* Avoid star import

* Need braces for control flow statements

* Redundant imports

* Add NewLineAtEndOfFile check
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
* Add Checkstyle framework

* Avoid star import

* Need braces for control flow statements

* Redundant imports

* Add NewLineAtEndOfFile check
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.

4 participants