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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla18
People
(Reporter: dholbert, Assigned: gps)
Details
Attachments
(1 file, 1 obsolete file)
5.88 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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 | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•