Refactor the python checkstyle plugin system.

Review Request #3061 — Created Oct. 30, 2015 and submitted

benjyw
pants
2209f27...
pants-reviews
molsen
Now instead of the plugins knowing about their corresponding
subsystems, the subsystems know about (and create) the plugins.

This allows us to only import (and therefore parse and interpret)
code we're actually going to use.  Performance profiles show that
almost a second of startup time is spent just loading backends,
and checkstyle was a good chunk of that.

If we like how this works, we can contemplate something similar
for backend code in general. Right now we load huge amounts of code
even if we never invoke it.

This change also creates some useful unittest infrastructure for
testing these checkstyle plugins. As a result it switches all
the relevant tests to use our idiomatic style (self.assertTrue
instead of assert keyword, and so on).

CI passes: https://travis-ci.org/pantsbuild/pants/builds/88461495

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
BE
MO
  1. Awesome, thanks for looking into the performance Benjy. Looks reasonable to me. Seems like a good model to follow over all.

  2. I think it would be good to change this to an equal comparison like we discussed so the nits will get printed on failure.

    1. It's the self.assertEqual(0, len(nits)) that I was proposing I should change to self.assertEqual([], nits). It's not possible to compare nits directly if it isn't empty, because Nit instances don't implement __eq__, nor is it obvious how they should (what about line number, for example? We don't check for that in all cases.)

      The alternative is to add a more explicit error message, which I've done.

  3. 
      
BE
BE
  1. Thanks for the review Matt! Submitted as 8802c8c07af07a5fe81ba3f463ebdf1162a62e1b.

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

8802c8c07af07a5fe81ba3f463ebdf1162a62e1b

Loading...