Let's do it in another rb.

I've modified ide_gen.

will do

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

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.

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.

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

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.

fixed

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.

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.

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.

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.

conf and classpath restored

+1

+1

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.

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.

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.

removed

related to the classpath expansion section note I have above.

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.

done

done

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.

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.

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