795491 - when I invoke mach with a misspelled command, its error output is redundant and includes odd "u" prefixes

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Daniel Holbert [:dholbert]]

STR:
 Run ./mach warning-summary

(note that I've intentionally misspelled the command -- the correct command is uses plural "warnings" in "warnings-summary")

ACTUAL RESULTS: This is printed to my terminal:
=================
usage: mach [-h] [--settings FILENAME] [-v] [-l FILENAME] [--log-interval]
            
            {build,settings-list,settings-create,test,mochitest-plain,mochitest-chrome,mochitest-browser,xpcshell-test,warnings-summary,warnings-list}
            ...
mach: error: argument action: invalid choice: 'warning-summary' (choose from u'build', u'settings-list', u'settings-create', u'test', u'mochitest-plain', u'mochitest-chrome', u'mochitest-browser', u'xpcshell-test', u'warnings-summary', u'warnings-list')
=================

EXPECTED RESULTS:
 * No duplicate list of commands (there are 2 lists of the build,settings-list, etc. commands above)
 * No weird u'...' prefixes on the options. (in the second list above)

Comment 1

[Daniel Holbert [:dholbert]]

Maybe the first list of options is supposed to be part of the "usage: ..." line?  If so, that's not immediately clear, to me at least, due to the blank line between "usage" and that list.

Comment 2

[Gregory Szorc [:gps]]

Removing the duplicate list of commands is definitely on my todo list. I think it requires reimplementing parts of argparse, sadly. I could be wrong. Maybe a Python guru knows better than me.

Again, blame Python on the u'' prefixes. I'm really not sure why it is doing that. Typically when you print() a unicode it drops the u'' prefix. Oh, Python.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Improve help text, v1

The monkeypatching is the least hacky and simplest way to fix this. The alternative would be implementing alternative classes for things in the argparse hierarchy. It would be a lot of work and I'm not sure if the payoff would be that good.

Anyway, the output now:

$ ./mach foo
Invalid command specified. The list of commands is printed below.

usage: mach [global arguments] command [command arguments]

Commands:
  {build,settings-list,settings-create,test,mochitest-plain,mochitest-chrome,mochitest-browser,xpcshell-test,warnings-summary,warnings-list}
    build               Build the tree.
    settings-list       Show available config settings.
    settings-create     Print a new settings file with usage info.
    test                Perform tests.
    mochitest-plain     Run a plain mochitest.
    mochitest-chrome    Run a chrome mochitest.
    mochitest-browser   Run a mochitest with browser chrome.
    xpcshell-test       Run an individual xpcshell test.
    warnings-summary    Show a summary of compiler warnings.
    warnings-list       Show a list of compiler warnings

Global Arguments:
  -h, --help            Show this help message and exit.
  --settings FILENAME   Path to settings file.
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Improve help text, v2

Now with that silly {foo,bar} list stripped from the help text:

$ ./mach foo
Invalid command specified. The list of commands is below.

usage: mach [global arguments] command [command arguments]

Commands:
    build               Build the tree.
    settings-list       Show available config settings.
    settings-create     Print a new settings file with usage info.
    test                Perform tests.
    mochitest-plain     Run a plain mochitest.
    mochitest-chrome    Run a chrome mochitest.
    mochitest-browser   Run a mochitest with browser chrome.
    xpcshell-test       Run an individual xpcshell test.
    warnings-summary    Show a summary of compiler warnings.
    warnings-list       Show a list of compiler warnings

Global Arguments:
  -h, --help            Show this help message and exit.
  --settings FILENAME   Path to settings file.
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.

Comment 5

[Jeff Hammel]

Comment on attachment 666131 [details] [diff] [review]
Improve help text, v2

+    def format_help(self):
+        text = argparse.ArgumentParser.format_help(self)
+
+        # Strip out the silly command list that would preceed the pretty list.
+        #
+        # Commands:
+        #   {foo,bar}
+        #     foo  Do foo.
+        #     bar  Do bar.
+        search = 'Commands:\n  {'

This is pretty hacky :( However, I don't know a better way of doing this OTTOMH. Might be worth noting that this is pretty fragile vs e.g. using an actual API

Comment 6

[Gregory Szorc [:gps]]

I realize it is fragile. I looked at the source of argparse and unfortunately there is no formal API for formatters aside from the high-level "format_help" and "format_usage" functions. While I would like to change specific low-level functions (e.g. the one that does the formatting for a "choices" list), since this isn't part of the formal API, this would be "extremely hacky" rather than "pretty hacky" so I decided against it.

Long term I envision this culminating with us writing our own full-fledged formatter (I'm pretty sure that's what Mercurial did). But, I'd rather not cross that bridge just yet.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/573907d351e3