Closed Bug 795491 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox Build System :: Mach Core, enhancement)

x86_64
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: gps)

Details

Attachments

(1 file, 1 obsolete file)

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)
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.
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.
Attached patch Improve help text, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #666128 - Flags: review?(jhammel)
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.
Attachment #666128 - Attachment is obsolete: true
Attachment #666128 - Flags: review?(jhammel)
Attachment #666131 - Flags: review?(jhammel)
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
Attachment #666131 - Flags: review?(jhammel) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: