Improve cmd-line help output. #docfixit

Review Request #2599 — Created Aug. 11, 2015 and submitted

benjyw
pants
5d7ad7f...
pants-reviews
jsirois, zundel

- Only print help for the scopes the user asked for (and anything
under them). Previously we printed help for all tasks under any
goals the user specified. E.g., `./pants help compile.java` would
print all compile help, including those for other tasks.
- Print help for subsystems, if asked. E.g., ./pants help cache
now shows help for global cache options. This falls nicely out of the
implementation of the point above.
- Be smarter about detecting whether a cmd-line token is a target spec
or an unrecognized scope, so we can issue an appropriate error in
the second case.
- Print the first line of an optionable's docstring in the help.

TODO: The subsytem help printing still needs some work. For example
you can do 'pants help jvm' and get help for the global jvm
subsystem, even though nothing uses it. John's pending change
to have subsystems know what scope they're embedded in should
help here.

TODO: This no longer prints the goal description before the help for all
options for tasks under the goal. But that can easily be restored
by making Goal an Optionable with a description (but, presumably,
no registered options). Will follow up with this soon.

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

BE
BE
ZU
  1. It makes me a bit uneasy that some seemingly unrelated tests were changed.

    1. See below- all those changes were necessary.

  2. s/criticaloperations/critical operations/

    1. This is fixed in another change I have out: https://rbcommons.com/s/twitter/r/2602/

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

    Seems a bit awkward to use an instance test here to me, but all I can think of is to suggest using constant values instead, probably not any better.

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

    What's up with this?

    1. Oops, that needed to go away. deleted.

  5. Did something break here?

    1. This, and the other similar test changes (ensuring that specs contain either a / or a : so they can be unambiguously detected as specs) is needed because the change at arg_splitter.py:191, described in the third bullet in the RB description. Previously we assumed that any token we didn't recognize as a scope was intended to be a target spec. This means that we could essentially never say "invalid goal", which wasn't very user-friendly. Now we check for a /, a : or an existing directory with the same name, so we can be more helpful.

      However this required changing the tests so that the things previously assumed to be specs are still treated as specs.

    2. There is one case that I want to make sure doesn't get broken again, which is the case of a spec that has no / or :. All of our BUILD files that users target are one or two directories deep, so the most common case in our repo is

      ./pants compile foo
      

      that runs the default target in the build file named foo/BUILD

    3. I patched this in and ran a few targets. Everything seems fine.

    4. Yeah, I eventually remembered that this was why the naive check wasn't enough, but checking for the presence of the directory seemed like a reasonable heuristic. Glad it worked!

  6. What's the reason for this change?

  7. Not sure why you made this change. Does it fail if you leave off the :tgt?

    Differentiating goals from targets is hard. an argument with a ':' is unambiguously a spec, but one without is harder to parse, maybe its a good test case. Maybe just omit it from one of the arguments "morts:tgt fleem' ?

  8. 
      
BE
JS
  1. LGTM mod missing new tests for no goals and unknown goals detection by the splitter.
    1. Forgive my dups to some of Eric's comments - missed those.
  2. src/python/pants/option/arg_splitter.py (Diff revision 2)
     
     

    I know you really dislike the idea of extending AbstractClass, but this will be a roadbump in moving to 3:

    >>> class Bob(object):
    ...   __metaclass__ = abc.ABCMeta
    ...   @abc.abstractmethod
    ...   def fred(self):
    ...     pass
    ... 
    >>> Bob()
    <__main__.Bob object at 0x7f51f7eb2898>
    >>> class Bob(object, metaclass=abc.ABCMeta):
    ...   @abc.abstractmethod
    ...   def fred(self):
    ...     pass
    ... 
    >>> Bob()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: Can't instantiate abstract class Bob with abstract methods fred
    
    1. ... but actually the ABCMeta is never in play (except as doc ) since you declare no @abstractmethod or @abstractproperty.  Maybe just ditch it.
    2. ... and the missing context was the repl session above was in a python3 repl.
    3. Oh I don't mind extending AbstractClass nearly as much now that it's in pants. I just forgot about it. I can use that as documentation.

  3. Not yours, but needs a space: critical operations
  4. My head is spinning with the oshi vs ohis vs ohi but I have no super suggestions (they do match the class names after all).  Maybe oshi -> scope_info, ohis -> infos, ohi -> info
  5. src/python/pants/option/options.py (Diff revision 2)
     
     
    UnknownGoalHelp and NoGoalHelp should probably trigger a non-zero exit in addition to printing help, but the signal is lost in the current bool return value.  This could certainly be a TODO for a follow-up RB.
  6. src/python/pants/option/options.py (Diff revision 2)
     
     
    Kill this block
  7. UnknownGoalHelp and NoGoalHelp should really get test coverage added here.
  8. 
      
BE
ST
  1. Great improvement! printing only the specified scope is particularly help-ful (ha)

    Print help for subsystems, if asked. E.g., ./pants help cache now shows help for global cache options. This falls nicely out of the implementation of the point above.

    This is great, thanks. One additional thing that would be nice though would be to have the subystem options included in ./pants help compile.zinc, for example. If alphabetized, they would naturally be sorted to the end.

    If you think of cache or jvm options, it's always helpful for them to be displayed in context.

    1. Yeah, I'll look into this. Trouble is, it does require the help system to differentiate between tasks and subsystems, but I suppose that's inevitable, since the user must differentiate.

  2. Mind doing the same for ZincCompile?

  3. Split

  4. 
      
JS
  1. Ship It!
  2. 
      
ZU
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as ef7c8b9ccfe4eeefd97d639fcf7593fc88edb9e2.

Loading...