Reimplement help formatting.

Review Request #2458 — Created July 10, 2015 and submitted

benjyw
pants
4355105...
pants-reviews
stuhood
Using the argparse help formatter was no longer viable: we had
to customize it in so many ways it was easier to simply implement
our own. The new implementation:

- Supports subsystem args.
- Separates help info extraction from rendering, which allows
  more code reuse (and less hackiness) in builddict and bash
  completion generation tasks.
- Is better tested than the old code.
- Uses ANSI colors for readability.
- Sensibly handles recursively-registered options (it displays
  them in basic help on the outermost scope the option was
  registered on, but then only in advanced help in the inner
  scopes that inherited the registration via recursion).

This change also makes the implementations of builddict and bash
completion generation simpler and supportive of subsystem options.

This implementation requires knowledge of what a scope represents
(a task, goal, subsystem etc.) and also of who registered each
option on a scope. Note that these overlap but are not identical.
For example, a recursive global option is registered by the global
registrar on all task scopes.

This makes a lot of options code quite a lot simpler, so I took
the opportunity of this refactoring to simplify even further.
For example, we no longer have separate paths for registering
boolean and non-boolean options.

I also deleted a couple of unimportant builddict tests, which
relied more on incidental behavior than on intended properties
of that code.  It was going to be too much effort to get them
to pass (as TestOptions don't expose necessary methods, and it
would be overkill to add them just to support these trivial tests).

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

A previous run had one odd test failure which I'm currently investigating.

BE
ST
  1. Awesome work sir. This looks like a fantastic cleanup, and the help output is drastically more usable.

  2. Sounds reasonable.

    1. Including the recursive ones, we have ~1500 scoped options...

    1. Fixed. I was getting tired by the end...

  3. should this use ScopeInfo instead? I suppose things generally shouldn't be both tasks and subsystems, but...

    1. The comment below was supposed to explain this but clearly did a bad job, so I rephrased and moved the location of the comment. Is this more helpful?

      # Autocomplete to this option in the enclosing goal scope, but exclude options registered
      # on us, but not by us, e.g., recursive options (which are registered by
      # GlobalOptionsRegisterer).
      # We exclude those because they are already registered on the goal scope anyway
      # (via the recursion) and it would be confusing and superfluous to have autocompletion
      # to both --goal-recursive-opt and --goal-task-recursive-opt in goal scope.
      if issubclass(ohi.registering_class, TaskBase):
        goal_scope = oschi.scope.partition('.')[0]
        autocomplete_options_by_scope[goal_scope].update(ohi.scoped_cmd_line_args)
      
  4. Attach a github issue # rather than a username?

    1. Yes, good idea. However I do want tom to fix it, as this is his baby, so I changed the TODO to point to the issue, but left his name there.

      See https://github.com/pantsbuild/pants/issues/1793.

  5. This looks awesome. Nice work!

  6. src/python/pants/option/parser.py (Diff revision 1)
     
     
     

    indentation

  7. pre-order depth first

  8. src/python/pants/option/scope.py (Diff revision 1)
     
     

    Are intermediate scopes defined somewhere? They're the only ones I don't recognize.

    1. They are the scopes we add in Options.complete_scopes() so we have a full hierarchy. E.g., if we register options on foo.bar this makes sure that foo exists too. Added a comment to that effect.

  9. 
      
BE
  1. Thanks for the quick review! This was probably as painful to review as it was to write... But I think we're going to enjoy this code in many ways.

  2. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 6376bc98b423073f22112e7307852ff472d4b165.

Loading...