mozprofile ignores arugments silently

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jeff Hammel, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Mistakenly I tried:

mozprofile /home/jhammel/mozmill/src/mozbase/mozprofile/tests/addons/empty.xpi

It creates a profile but doesn't add the addon.  This isn't surprising
as the --addon switch is used to specify the addon, not just a
free-form argument.

that said, we don't do anything with free form arguments.  If we don't
want to utilize them, we should at least err out if they are provided,
since failing silently is, at best, confusing.

Comment 1

4 years ago
Created attachment 787129 [details] [diff] [review]
0001-Bug-871791-added-display-of-help-when-user-supplied-.patch

Comment 2

4 years ago
Hi, i think it's best to display help if arguments aren't supported. IMO it's very common behaviour in console programs.
Unfortunately, i'm not sure about testing cli-commands in mozbase code (i couldn't find example). If this is required, I can add next patch with proper test.
(Reporter)

Comment 3

4 years ago
Comment on attachment 787129 [details] [diff] [review]
0001-Bug-871791-added-display-of-help-when-user-supplied-.patch

This looks good to me.  I don't see any need for tests for this particular bug at this point.
Attachment #787129 - Flags: review?(ahalberstadt)
Comment on attachment 787129 [details] [diff] [review]
0001-Bug-871791-added-display-of-help-when-user-supplied-.patch

Review of attachment 787129 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: mozprofile/mozprofile/cli.py
@@ +105,5 @@
>  
> +    if cli.args:
> +        print "Program doesn't support positional arguments."
> +        cli.parser.print_help()
> +        return

I would just use cli.parser.error() here, but this is ok too.
Attachment #787129 - Flags: review?(ahalberstadt) → review+
Programs should really not return 0 if they are exiting with an error condition, as we are here. The "return" should be either changed to a cli.parser.error() or a sys.exit(1).

Comment 6

4 years ago
Wiliam Lachance: 
Totally right, i'll change this return into sys.exit(-1).
(Reporter)

Comment 7

4 years ago
IMHO it is contentious whether programs that require a positional argument should exit 0 or 1 (or other).  There are plenty of examples of both in the unix world.  That said, if we are counting this as an error we should use parser.error() as Andrew and Will suggest and probably document this requirement for mozbase.

(FWIW, I generally make my programs exit 0 and display usage (.print_help() in OptionParser) but that is not advocacy for it in mozbase)

Comment 8

4 years ago
Created attachment 789794 [details] [diff] [review]
0001-Bug-871791-added-display-of-help-when-user-supplied-.patch

Ok, I'm uploading new version with cls.parser.error. I think calling program with bad parameters should be noticeable on both levels (system's and user's), so it's important to return proper exit codes in shell environment.
Attachment #787129 - Attachment is obsolete: true
(Reporter)

Comment 9

4 years ago
Comment on attachment 789794 [details] [diff] [review]
0001-Bug-871791-added-display-of-help-when-user-supplied-.patch

reviewing per irc request http://logbot.glob.com.au/?c=mozilla%23ateam#c664869
Attachment #789794 - Flags: review+
Pushed: https://github.com/mozilla/mozbase/commit/5f5451af842ed88cd890e4d128c0d63ea6a434b7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.