mozprofile ignores arugments silently



6 years ago
5 years ago


(Reporter: k0scist, Unassigned)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



6 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.
Created attachment 787129 [details] [diff] [review]
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.

Comment 3

5 years ago
Comment on attachment 787129 [details] [diff] [review]

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]

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

Thanks for the patch!

::: mozprofile/mozprofile/
@@ +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).
Wiliam Lachance: 
Totally right, i'll change this return into sys.exit(-1).

Comment 7

5 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)
Created attachment 789794 [details] [diff] [review]

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

Comment 9

5 years ago
Comment on attachment 789794 [details] [diff] [review]

reviewing per irc request
Attachment #789794 - Flags: review+
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.