Improve the implementation of help printing.

Review Request #1744 — Created Feb. 10, 2015 and submitted

benjyw
pants
84cc2fb...
pants-reviews
areitz, jsirois, lahosken, zundel

- Encapsulates the various help request options (advanced, all)
in a HelpRequest object.
- Create two help formatter classes, one for basic help and one for advanced help,
to avoid futzing with global state.
- Moves all help-printing logic into Option. Previously some of it
had leaked out into GoalRunner.

This is more-or-less what I had in mind in my comments on https://rbcommons.com/s/twitter/r/1739/ (although this is a little bit wider scope).

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

Ran various help cmd-lines and verified that they printed what I expect.

  • 0
  • 0
  • 0
  • 2
  • 2
Description From Last Updated
JS
  1. 
      
  2. src/python/pants/option/arg_splitter.py (Diff revision 1)
     
     
    If you used kwargs this would be a surprising instantiation, how about HelpRequest(False, ()) or else add a classmethod to HelpRequest to produce a default instance.
    
    There is another instance of this down on line 150.
    1. HelpRequest has two fields, each of boolean type. HelpRequest(False, False) is exactly how I would expect to instantiate it. It's equivalent to HelpRequest(advanced=False, all_scopes=False). HelpRequest(False, ()) would assign an empty tuple to 'all_scopes', which is not what we want.

      However I've created a classmethod as you suggest, for clarity.

    2. Aha - I read poorly and expected all_scopes was a list of scope names.  Larry's doc prompt helped here.
  3. I always prefer an abstract property for this sort of thing - reads a fair bit cleaner.
    1. Introducing the ABC machinery here seems like overkill to me, tbh.

  4. 
      
ZU
  1. 
      
  2. src/python/pants/option/arg_splitter.py (Diff revision 1)
     
     

    (This is where --help-advanced was disappearhing before.)

  3. src/python/pants/option/options.py (Diff revision 1)
     
     

    How about documenting --help-all as an option? It is a bit confusing that in the help below its presented like a goal, but there is no goal registered.

    What do you think about adding it to options_bootstrapper.py like I did for --help-advanced? Or did you mean to remove --help-advanced from the bootstrap options?

    1. I documented it as a "goal" to mirror how we document 'help' immediately above it. I think as long as we document some way of getting help, we don't need to be exhaustive about showing every possible way. I'm fairly agnostic as to whether the way we document is 'goal style' or 'flag style', I just stuck with what we were already doing.

      Either way, I did intend to remove --help-advanced from the boostrap options, as it's now a special-case, not a real option.

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

    If this is the way you are going to document the help, should you add ./pants help-advanced? (I don't really think so, but it would be consistent with what we have)

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

    I like this better.

  6. 
      
LA
  1. 
      
  2. src/python/pants/option/arg_splitter.py (Diff revision 1)
     
     

    A docstring would be nice. If I'm understanding correctly, that docstring might be

    How much help the user asked for.

    ...or...

    Which help-related options the user passed

    ...but maybe I guessed wrong.

  3. 
      
BE
ZU
  1. I would prefer to see --help-all and --help-advanced documented as -- style options, but as long as there is some way to get the help we want (and the other methods are shown right at the top of the output), it is just a nit.

  2. 
      
AR
  1. I checked this out and played around with it. Overall, it looks good -- things like this work now:

    [prometheus pants (tmp)]$ ./pants publish --help-advanced
    
    publish: Publish artifacts.
    
    publish
      --publish-help
      -h, --help              show this help message and exit
      --publish-nailgun-server=_PUBLISH_NAILGUN_SERVER__
    ...
    

    (i.e. no need to specify --help --help-advanced). But in my poking around, I don't see pants advertise the "help-advanced" flag anywhere. For example:

    [prometheus pants (tmp)]$ ./pants -h
    Pants 0.0.28 https://pypi.python.org/pypi/pantsbuild.pants/0.0.28
    
    Usage:
      ./pants [option ...] [goal ...] [target...]  Attempt the specified goals.
      ./pants help                                 Get help.
      ./pants help [goal]                          Get help for a goal.
      ./pants help-advanced [goal]                 Get help for a goal's advanced options.
      ./pants help-all                             Get help for all goals.
      ./pants goals                                List all installed goals.
    
      [target] accepts two special forms:
        dir:  to include all targets in the specified directory.
        dir:: to include all targets found recursively under the directory.
    
    Friendly docs:
      http://pantsbuild.github.io/
    
    Global options:
    -h, --help              show this help message and exit
    --pants-workdir <dir>   Write intermediate output files to this dir. (default:
                            /Users/areitz/workspace/pants/.pants.d)
    ...
    

    Pants says you can do ./pants help-advanced, and mentions the --help flag, but never mentions --help-advanced. It seems like maybe it should?

    Also, when I do ./pants help-all, it doesn't show me the advanced options. However, ./pants help-all --help-advanced does. This might be a reasonable thing to do, but what do others think?

    1. The sentiment driving advanced in the first place was to hide these things because user's would do crazy and they are basically never needed.  I think there is plenty of info in the help as it stands to get at these advanced options.
  2. 
      
NH
  1. 
      
  2. src/python/pants/option/arg_splitter.py (Diff revision 2)
     
     

    Could you break this up into Query / Command methods?
    ie

    def _is_help_arg(self, arg):
    def _update_help_request(self)
    

    I would prefer if state changes were separated from state queries, especially in the parser.

  3. src/python/pants/option/options.py (Diff revision 2)
     
     

    Could you break this up into Query / Command methods?
    ie

    def help_requested(self):
    def print_help(self)
    
  4. 
      
IT
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 1990469be04642458413f467037dc32811705ca1.

Loading...