Simplify phase.py.

Review Request #856 — Created Aug. 8, 2014 and submitted

benjyw
pants
pants-reviews
jsirois
- Gets rid of clever but complicated deduping logic. Replaces it with
  dumb but simple deduping logic via a factory.

- Gets rid of a few methods that were either entirely unused or only used
  once (in which case replaced them with alternative code).

- Phases now store task names and types directly. The TaskRegistrar instance
  isn't retained after registration.

- A Phase now directly knows its depenendcies on other phases, instead of
  delegating to the union of dependencies of its TaskRegistrars.
  This makes a lot of code much simpler.

- Phase no longers uses the old 'goal' terminology.

- Move logic such as option parser setup from TaskRegistrar into Phase.

This simplifies Phase itself and a lot of code that uses it, and
paves the way for renaming Phase to Goal.
CI passed: https://travis-ci.org/pantsbuild/pants/builds/32051833. 

Unit and integration tests pass locally too.

Ran some manual builds, including "./pants goal goals --goals-graph".
JS
  1. Happy to see this.  I'll give this a good review in the afternoon/evening if it can wait until then.
  2. 
      
JS
  1. 
      
  2. src/python/pants/goal/phase.py (Diff revision 1)
     
     
    NotImplementedError or TypeError (ala the abc module) - seem slight better scoped.
    1. Changed to TypeError.
  3. src/python/pants/goal/phase.py (Diff revision 1)
     
     
    I actually like Phases, but you ended up spelling it Phase, so the doc needs to be fixed here.
  4. src/python/pants/goal/task_registrar.py (Diff revision 1)
     
     
    This method is broken, but its also dead code, so kill.
    1. Good catch. Killed.
  5. 
      
BE
JS
  1. Ship It!
    1. Shipped (via ./rbt patch). Thanks!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

ZU
  1. 
      
  2. FYI pants goal builddict is now broken after this commit.
    1. We should really make that an integration test, so a CR can't get through CI if it breaks docgen.
    2. less we more me: https://rbcommons.com/s/twitter/r/873/
    3. John Sirois fixed the crash, YAY. But BUT
      --help and 'goal builddict' don't "find" many flags.
      
      
      $ ./pants goal resolve -h
      
      resolve: Resolve dependencies and produce dependency reports.
      
      resolve options:
        --ng-daemons, --no-ng-daemons
                                [True] Use nailgun daemons to execute java tasks.
      $
      
      this affects -h and 'goal builddict'. https://github.com/pantsbuild/pants/issues/478
      
      I'll look at this, but if a smartie swoops in with the answer on a silver platter, that's probably better :-)
    4. It was a code shuffle miss of 2 lines.  I'll send a review.
    5. https://rbcommons.com/s/twitter/r/877/
  3. 
      
Loading...