Closed
Bug 1076649
Opened 10 years ago
Closed 10 years ago
Remove the '+' prefixing from e.g. mach run arguments
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 2 obsolete files)
19.03 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 1•10 years ago
|
||
The reason to use '+' prefixing was to distinguish between options to the mach command itself, and options that are passed down to whatever the command does (like mach run passing down args to the built application). That makes things unnecessarily awkward, and quite non-standard. Instead, use standard '-' prefixing, and pass all the unknown arguments down. If there is overlap between the known arguments and arguments supported by the underlying tool (like -remote when using mach run), it is possible to use '--' to mark all following arguments as being targetted at the underlying tool. For instance: mach run -- -remote something would run firefox -remote something while mach run -remote something would run firefox something As allow_all_arguments is redundant with the presence of a argparse.REMAINDER CommandArgument, allow_all_arguments is removed. The only mach command with a argparse.REMAINDER CommandArgument without allow_all_arguments was "mach dmd", and it did so because it didn't want to use '+' prefixes. Note I didn't want to add a specific error message in case someone passes a '+' prefixed argument. It should be enough that the option is rejected. People can look at --help and figure that it's not '+' anymore.
Attachment #8498644 -
Flags: review?(gps)
Comment 2•10 years ago
|
||
Comment on attachment 8498644 [details] [diff] [review] Remove the '+' prefixing from mach commands with allow_all_arguments=True Review of attachment 8498644 [details] [diff] [review]: ----------------------------------------------------------------- You missed an occurrence of 'allow_all_arguments' in python/mach/mach/base.py, as far as I can tell.
Assignee | ||
Comment 3•10 years ago
|
||
The reason to use '+' prefixing was to distinguish between options to the mach command itself, and options that are passed down to whatever the command does (like mach run passing down args to the built application). That makes things unnecessarily awkward, and quite non-standard. Instead, use standard '-' prefixing, and pass all the unknown arguments down. If there is overlap between the known arguments and arguments supported by the underlying tool (like -remote when using mach run), it is possible to use '--' to mark all following arguments as being targetted at the underlying tool. For instance: mach run -- -remote something would run firefox -remote something while mach run -remote something would run firefox something As allow_all_arguments is redundant with the presence of a argparse.REMAINDER CommandArgument, allow_all_arguments is removed. The only mach command with a argparse.REMAINDER CommandArgument without allow_all_arguments was "mach dmd", and it did so because it didn't want to use '+' prefixes.
Attachment #8499443 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8498644 -
Attachment is obsolete: true
Attachment #8498644 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8499443 -
Attachment is obsolete: true
Attachment #8499443 -
Flags: review?(gps)
Comment 5•10 years ago
|
||
Comment on attachment 8499519 [details] [diff] [review] Remove the '+' prefixing from mach commands with allow_all_arguments=True Review of attachment 8499519 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thank you for scratching this UX wort.
Attachment #8499519 -
Flags: review?(gps) → review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8f29e386bd
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4e8f29e386bd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•