Closed Bug 1076649 Opened 5 years ago Closed 5 years ago

Remove the '+' prefixing from e.g. mach run arguments

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
OS: Linux → All
Hardware: x86_64 → All
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 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.
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)
Attachment #8498644 - Attachment is obsolete: true
Attachment #8498644 - Flags: review?(gps)
Attachment #8499443 - Attachment is obsolete: true
Attachment #8499443 - Flags: review?(gps)
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+
https://hg.mozilla.org/mozilla-central/rev/4e8f29e386bd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.