Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show usage on missing or wrong command. #311

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Conversation

johannhof
Copy link
Contributor

It's pretty annoying that you only get a stack trace when simply running web-ext without any arguments for the first time, so I added help messages.

@coveralls
Copy link

coveralls commented Jun 22, 2016

Coverage Status

Coverage increased (+0.0007%) to 99.818% when pulling 2a54587 on johannhof:master into 03b1b7a on mozilla:master.

@kumar303
Copy link
Contributor

I think it's better to do this with yargs.strict(). Could you try it? It was started here but abandoned: #169 Also, it needs to work for incorrect sub-command arguments. Example: web-ext build --not-an-option should show an error.

@kumar303
Copy link
Contributor

Were you trying on web-ext 1.0.1 or one built against master? When I try against master it's pretty good but it needs this patch to handle sub-command options:

diff --git a/src/program.js b/src/program.js
index 7b21b16..2f03ae4 100644
--- a/src/program.js
+++ b/src/program.js
@@ -37,7 +37,7 @@ export class Program {
       }
       // Calling env will be unnecessary after
       // https://github.com/yargs/yargs/issues/486 is fixed
-      return yargs.env(envPrefix).options(commandOptions);
+      return yargs.strict().env(envPrefix).options(commandOptions);
     });
     this.commands[name] = executor;
     return this;

@kumar303
Copy link
Contributor

^ that causes the test process to exit though because yargs exits the process. My idea for dealing with that was: #169 (comment)

@johannhof
Copy link
Contributor Author

@kumar303 pushed another approach which works for me, anything wrong with that one? Tests seem to be passing.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage remained the same at 99.827% when pulling f6a2a26 on johannhof:master into fd2c4d6 on mozilla:master.

@johannhof
Copy link
Contributor Author

The thing that looks ugly on master for me is simply typing web-ext without any sub-commands, but now it should look good in all cases.

@kumar303
Copy link
Contributor

Thanks! Looks good and the patch works for me. It doesn't address #169 but we can do that in a separate patch (it's a related but separate issue).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants