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.

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   Let's do it in another rb.

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.

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   I've modified ide_gen.

pants.ini (Diff revision 2)

Since supression_files has been renamed, add an entry for it in src/python/pants/option/migrate_config.py

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   will do

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?

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   take a look https://rbcommons.com/s/twitter/r/1145/ look for John's comments

2. 

   John Sirois Dec. 8, 2014, 5:43 p.m.

   I'm not sure if more explanation is needed, Eric has since created a page describing setting up a small plugin setup in a new repo, but even after we seperate the current jvm backend into a java backend such that a java project can just install the java backend, forcing checkstyle on means we pick a default style.  We could reasonably pick Sun style and express an opinion forcing folks to turn off or re-configure the style in-play, but it seems to me the build tool shouldn't force language styles, it should just make it easy to turn enforcement on.  Today turning a plugin on takes more than a single pants.ini line; however, given fine-grained plugin distribution and David Taylor's current work on plugin resolution, it would just be a single line containing the checkstyle plugin pypi requirement to fetch and activate the plugin.  A second line to enable canned styles - say Sun or Google, and more lines if the user wanted to setup a custom checkstyle config.

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.

1. 

   John Sirois Dec. 8, 2014, 5:43 p.m.

   See http://checkstyle.sourceforge.net/config_coding.html 'RedundantThrows'.  That check was historically used by Twitter and requires a classpath with all pussible exception base classes.  That should explain the 'resolve' dep - and the 'gen' dep is there, because, for example, an exception thrown from an RPC might very reasonably extend org.apache.thrift.TApplicationException which is a synthetic runtime dep for thrift java gen targets injected by pants in the gen phase just before resolve.

src/python/internal_backend/optional/register.py (Diff revision 2)

did you mean to revert th description here?

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   Yes. the description becomes the compile description, no the scalastyle description once I thought it was. It looks weird and wrong.

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.

1. 

   Jin Feng Dec. 5, 2014, 2 p.m.

   this is ide_gen.py. I didn't change idea_gen.py.
   I agree idea and checkstyle should share something, that's what I wrote in the comment, that I'll follow up with Tejal.

tests/python/pants_test/backend/jvm/tasks/test_checkstyle.py (Diff revision 2)

Nice tests! thanks.

src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 3)

Show all issues

Use parens around the entire expression rather than backslashes.  Also, the and trails the expression rather than going on the next line.

1. 

   Jin Feng Dec. 5, 2014, 7:34 p.m.

   fixed

src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4)

Show all issues

are we removing/keeping this then?

1. 

   John Sirois Dec. 8, 2014, 5:58 p.m.

   It should be kept - at least partially.  There are 2 mainly orthoganal things going on here, confs and classpath augmentation.  Lets ignore confs to start - the egroups stuff is all there to pluck out the classpath of the code being checkstyled and that's needed for certain checkstyle rules.  The conf bit is secondary to that.  It will eventually go away completely, but may not be able to be factored out today.

2. 

   Jin Feng Dec. 10, 2014, 4:25 p.m.

   I'm not following:
   The original code is:
   
   egroups = self.context.products.get_data('exclusives_groups')
   etag = egroups.get_group_key_for_target(targets[0])
   classpath = self.tool_classpath(self._checkstyle_bootstrap_key)
   cp = egroups.get_classpath_for_group(etag)
   classpath.extend(jar for conf, jar in cp if conf in self.get_options().confs)
   
   Note in my diff, I've moved
   classpath = self.tool_classpath(self._checkstyle_bootstrap_key)
   
   to the top of the function, so that's still there. The rest seems to me all related to confs stuff: getting egroups -> getting etag -> getting cp for the etag -> adding those cp whose conf appears in confs argument. If we said confs is no longer relevant, I don't see why those lines need to be kept.

3. 

   John Sirois Dec. 10, 2014, 6:12 p.m.

   Its true confs are generally not relevant - we are trying to kill them, that said - forget about confs for a second - they truly are secondary here.
   
   There are classpaths:
   
   1. the tool classpath - this is needed to be able to run checkstlye at all, even just checkstyle -h
   
   2. the classpath of the code being checked
   Its the second classpath that is absolutely required by checkstyle rules like XXX.  Unless you want to remove support for ./pants goal checkstyle checking those sorts of rules, that classpath must still be present after this change.
   
   The fact that you still need confs to populate a classpath is an artifact, we'd like to remove it, but folks reading classpaths still need to deal with it.  In practice the confs always is the tuple ('default',) so the guard in the for comprehension here could likely be removed: classpath.extend(jar for conf, jar in cp if conf in self.get_options().confs).  That said - you are not here to shave the conf yak, so you I'd just leave the code as-is.

4. 

   John Sirois Dec. 10, 2014, 6:14 p.m.

   Where XXX is "See http://checkstyle.sourceforge.net/config_coding.html 'RedundantThrows'" - I talked about this example above in a reply to Eric's 1st feedback section.

5. 

   Jin Feng Dec. 10, 2014, 10:21 p.m.

   conf and classpath restored

src/python/pants/backend/jvm/tasks/ide_gen.py (Diff revision 4)

Show all issues

remove?

1. 

   John Sirois Dec. 8, 2014, 6:02 p.m.

   +1

src/python/pants/option/migrate_config.py (Diff revision 4)

Show all issues

remove?

1. 

   John Sirois Dec. 8, 2014, 6:02 p.m.

   +1

build-support/checkstyle/suppressions.xml (Diff revision 4)

If we need no suppressions today - why allow them at all in the pants repo?

1. 

   Jin Feng Dec. 10, 2014, 4:32 p.m.

   My thought is to show how people can use it. If we remove the suppresssions.xml, then the properties and 'checkstyle.suppression.file' setting would be removed from pants.ini as well. Not necessarily straightforward for people to figure out.

2. 

   John Sirois Dec. 10, 2014, 6:23 p.m.

   OK - categorical imperative again - not sure of the answer, but do we want to express every default for every task in pantsbuild/pants own config?  This could have the effect of people knowing the now-hidden customization incantations, but not knowing they are optional.  In otherwords they are very likely to cargo-cult the config when they do not need to.  The primary gate to pants 1.0 is still that a user need only touch pants.ini in their repo after installing pants with pip globally.
   It seems to me we need 2 pieces of information for good examples, what is optional configuration if you need to go custom, what is required configuration.  Now pantsbuild/pants will always be central - if folks have a question about pants setup, they'll likely come to this repo and see the example config.  Plugin writers though do not share this priviledge of prime location - no one will bother finding their plugin repo to look at its maximal config for the 1 task the plugin exports.  Much much better is providing a pluggable way to allow a pants user to discover this information uniformly.  We have the command line help today and that can be improved, but it treats pants core and random plugin equally, all options are displayed the same.  For higher level docs / examples we have no established solution.  It could be that ./pangt goal dictionary <task name|target name> gets built and that pops up docs in a browser.  Those docs might be embedded in plugins in the form of markdown resource files in some uniform way - I'm not sure.
   Either way - I am of the opinion that doc examples will scale better than checked in examples even in the gap until someone writes the dictionary goal.

3. 

   Benjy Weinberger Dec. 10, 2014, 6:57 p.m.

   I am definitely of the opinion that pants should work out of the box with an empty pants.ini. The new options system was designed with this goal in mind. Everything can and should have a sensible default in the option registration.

4. 

   Jin Feng Dec. 10, 2014, 10:21 p.m.

   removed

src/python/pants/backend/jvm/tasks/checkstyle.py (Diff revision 4)

Nope - this is detritus to sweep away.

1. 

   Jin Feng Dec. 10, 2014, 4:32 p.m.

   related to the classpath expansion section note I have above.

2. 

   John Sirois Dec. 10, 2014, 6:23 p.m.

   Aha yes - I was wrong.  We do want to sweep away confs, but this code does in fact need them to access the classpath of the code being checked by checkstyle.

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?

1. 

   Jin Feng Dec. 10, 2014, 4:32 p.m.

   done

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.

1. 

   Jin Feng Dec. 10, 2014, 4:32 p.m.

   done

Thanks a bunch for the test.

src/python/internal_backend/optional/register.py (Diff revision 6)

Show all issues

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|

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.

1. 

   Jin Feng Dec. 15, 2014, 5:34 p.m.

   oh, there were two places hardcoded '.java' so i created a constant for it. it wasn't meant for general purpose use and i had no intention of subclassing neither.

2. 

   David Turner Dec. 15, 2014, 6:07 p.m.

   From my perspective (and maybe I'm wrong), it's OK to have two places that say ".java", because it's shorter and easier to understand than a constant.  Constants are good for things that might change (".java" -> ".scala"); things people won't remember (3.1415926535897932 vs PI); and things people won't understand without context (7 vs DAYS_IN_WEEK).  But for cases like this, I would prefer to skip the constant.
   This is totally a nit, tho, and it's OK to ship as-is.

3. 

   David Turner Dec. 15, 2014, 6:09 p.m.

   I'm actually just going to merge it as-is.  Please mark this as submitted.

Ship It!

Ship It!