Change Summary:
update a question comment
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+278 -47) |
Review Request #1432 — Created Dec. 5, 2014 and submitted
Information | |
---|---|
jinfeng | |
pants | |
fa8e473... | |
Reviewers | |
pants-reviews | |
benjyw, ity, jsirois, tejal, zundel |
- Renaming BUILD.tools's twitter-checkstyle target to checkstyle since this is not
twitter specific
- Adding build-support/checkstyle/* to wire up the checkstyle
- A few longer than 100-character per line java files fixes
- pants.ini, fix/change broken [checkstyle] to [compile.checkstyle]
- pants.ini remove suppression_files setting:
There are a cuople reasons for doing this:
1) OS pants and com.puppycrawl.tools.checkstyle only supports single suppression file.
[HTML_REMOVED] The multiple suppresion files concept is from a Twitter internal custom filter, which
[HTML_REMOVED] isn't applicable here.
2) But more important, suppression isn't a required component for running checkstyle.
[HTML_REMOVED] Someone can perfectly author a coding_style.xml which contains no SuppressionFilter
[HTML_REMOVED] filter. And right now the checkstyle internally reads suppression_files list and injects
[HTML_REMOVED] them into the properties, which assuming the hardcoded SuppressionFilter. Doesn't make
[HTML_REMOVED] any sense. Anyone wants to use suppression, can still put checkstyle.suppression.file
[HTML_REMOVED] into properties setting under [compile.checkstyle] in OS pants, or any company specific
[HTML_REMOVED] pants.ini
- Adding the checkstyle task into the optional backend so when we run PANTS_DEV=1 ./pants
in OS pants, it's added, however, it's not by default wired up in pants release. Anyone
can still wire it up in their own wrapper (like what Twitter currently does)
- In ide_gen, currently it tries to read suppression_files from [checkstyle]. First, like
I mentioned above, the default open source checkstyle doesn't have multiple suppression
files concept. Second, the current setting points to an invalid file that doesn't exist
anyway. So instead, I'm setting it to [] for now and will follow up with ide/idea owners
later.
- Adding unit tests for checkstyle to prevent future regressions.Merge branch 'master' of github.com:jinfeng/jinfeng-pants-fork into jin_add_checkstyle_to_optional_backend
PANTS_DEV=1 ./pants goal compile examples/src/java::
PANTS_DEV=1 ./pants goal test tests/python/pants_test/backend/jvm/tasks:checkstyleTravis baking:
https://travis-ci.org/jinfeng/jinfeng-pants-fork/builds/43051952
update a question comment
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+278 -47) |
BUILD.tools (Diff revision 2) |
---|
I would like to see if we update the version here and see that things still work? 5.6 is too old to support Java 8. 5.9 supports Java 8 and 6.1.1 is the latest. If this feels like scope creep I can follow on later.
pants.ini (Diff revision 2) |
---|
Note that 'ide_gen' also reads the [checkstyle] section. If you make a change here then you probably need to update that if you want to continue to get checkstyle support in the IDE.
pants.ini (Diff revision 2) |
---|
Since supression_files has been renamed, add an entry for it in src/python/pants/option/migrate_config.py
src/python/internal_backend/optional/register.py (Diff revision 2) |
---|
It doesn't make a lot of sense to me to have checkstyle defined in the core backend section, but only optionally wired up here. The internal_backend is really only intended for pants development.
Maybe there is some other way we could turn checkstyle on and leave it off by default, like a options 'enabled' or something that defaults to false?
src/python/internal_backend/optional/register.py (Diff revision 2) |
---|
I don't know why this would be needed today, but I'd like to hear the back story.
src/python/internal_backend/optional/register.py (Diff revision 2) |
---|
did you mean to revert th description here?
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 2) |
---|
I'm not sure anyone is using exclusives_groups support any more. Foursquare was using it but they've substituted something else.
src/python/pants/backend/jvm/tasks/ide_gen.py (Diff revision 2) |
---|
Note, these lines are copied from ide_gen.py. There is this relationship between ide and checkstyle that we need to sort out. It would be nice I think if Ide and Checkstyle provided some static method to return these config variables instead of dipping directly into config in case we need to move this configuration around.
address migrate_config.py comment.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+286 -49) |
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 3) |
---|
Use parens around the entire expression rather than backslashes. Also, the
and
trails the expression rather than going on the next line.
address style comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+286 -49) |
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4) |
---|
are we removing/keeping this then?
build-support/checkstyle/suppressions.xml (Diff revision 4) |
---|
If we need no suppressions today - why allow them at all in the pants repo?
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4) |
---|
Nope - this is detritus to sweep away.
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4) |
---|
The target logging here and the source logging below reresent a lot of logging and that logging is redundant to information in the build server UI which lists both targets expandable to sources as well as the cmdline which has sources. Can this all be killed in favor of the existing logging?
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4) |
---|
I think you already touch all % string interpolation formatting lines in this file - as such it would be good to -> {} / .format style interpolation.
addressed Ity's and most John's comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+264 -58) |
address john's comments: 1) conf/classpath 2) removal of suppression.xml
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+227 -51) |
Thanks a bunch for the test.
src/python/internal_backend/optional/register.py (Diff revision 6) |
---|
This still needs to be reverted - the resolve is needed for the code-being-checked classpath (for the few checkstyle rules that need a classpath). The gen is needed because it can contribute to the resolve classpath. In short - those deps are what provide egroups hits.
tests/python/pants_test/backend/jvm/tasks/test_checkstyle.py (Diff revision 6) |
---|
alpha - when doing import ordering, ignore the leading import or from.
tests/python/pants_test/backend/jvm/tasks/test_checkstyle.py (Diff revision 6) |
---|
Even though it works - its odd to read: s|src/java/|src/java|
Committers please take this in. Thanks!!
fixed john's additional comments.
merged with latest master
travis passed: https://travis-ci.org/jinfeng/jinfeng-pants-fork/builds/43844791
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+225 -51) |
src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 7) |
---|
I think it's OK to hard-code ".java". If you wanted to make this more general, then just call it _SOURCE_EXTENSION, so that subclasses could do some other language.