fix: Missing command/option errors are reported better

mozilla · Jun 24, 2016 · 33a9e01 · 33a9e01
1 parent 6d2e673
commit 33a9e01
Show file tree

Hide file tree

Showing 5 changed files with 187 additions and 100 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -30,6 +30,7 @@
     "dot-notation": [2, {"allowKeywords": true}],
     "eqeqeq": [2, "allow-null"],
     "guard-for-in": 0,
+    "indent": [2, 2],
     "keyword-spacing": 2,
     "max-len": [2, 80, 2, {ignoreComments: true}],
     "new-cap": [2, {"capIsNewExceptions": ["Deferred"]}],

diff --git a/Gruntfile.js b/Gruntfile.js
@@ -55,23 +55,23 @@ module.exports = function(grunt) {
   });
 
   grunt.registerTask(
-      'check-for-smoke',
-      'checks to see if web-ext is completely broken', function() {
-    grunt.log.writeln('making sure web-ext is not catastrophically broken');
+    'check-for-smoke',
+    'checks to see if web-ext is completely broken', function() {
+      grunt.log.writeln('making sure web-ext is not catastrophically broken');
 
-    var done = this.async();
-    var webExt = path.join(path.resolve(__dirname), 'bin', 'web-ext');
-    var result = spawn(webExt, ['--help']);
+      var done = this.async();
+      var webExt = path.join(path.resolve(__dirname), 'bin', 'web-ext');
+      var result = spawn(webExt, ['--help']);
 
-    result.stderr.on('data', function(data) {
-      grunt.log.writeln(data);
-    });
+      result.stderr.on('data', function(data) {
+        grunt.log.writeln(data);
+      });
 
-    result.on('close', function(code) {
-      grunt.log.writeln('web-ext exited: ' + code);
-      var succeeded = code === 0;
-      done(succeeded);
+      result.on('close', function(code) {
+        grunt.log.writeln('web-ext exited: ' + code);
+        var succeeded = code === 0;
+        done(succeeded);
+      });
     });
-  });
 
 };
diff --git a/src/firefox/remote.js b/src/firefox/remote.js
@@ -51,15 +51,15 @@ export class RemoteFirefox {
   addonRequest(addon: Object, request: string): Promise {
     return new Promise((resolve, reject) => {
       this.client.client.makeRequest(
-          {to: addon.actor, type: request}, (response) => {
-        if (response.error) {
-          reject(
-            new WebExtError(`${request} response error: ` +
-                            `${response.error}: ${response.message}`));
-        } else {
-          resolve(response);
-        }
-      });
+        {to: addon.actor, type: request}, (response) => {
+          if (response.error) {
+            reject(
+              new WebExtError(`${request} response error: ` +
+                              `${response.error}: ${response.message}`));
+          } else {
+            resolve(response);
+          }
+        });
     });
   }
 

diff --git a/src/program.js b/src/program.js
@@ -17,13 +17,15 @@ const envPrefix = 'WEB_EXT';
 export class Program {
   yargs: any;
   commands: { [key: string]: Function };
+  shouldExitProgram: boolean;
 
   constructor(argv: ?Array<string>, {yargsInstance=yargs}: Object = {}) {
     if (argv !== undefined) {
       // This allows us to override the process argv which is useful for
       // testing.
       yargsInstance = yargs(argv);
     }
+    this.shouldExitProgram = true;
     this.yargs = yargsInstance;
     this.yargs.strict();
     this.commands = {};
@@ -35,9 +37,13 @@ export class Program {
       if (!commandOptions) {
         return;
       }
-      // Calling env will be unnecessary after
-      // https://github.com/yargs/yargs/issues/486 is fixed
-      return yargs.env(envPrefix).options(commandOptions);
+      return yargs
+        .strict()
+        .exitProcess(this.shouldExitProgram)
+        // Calling env() will be unnecessary after
+        // https://github.com/yargs/yargs/issues/486 is fixed
+        .env(envPrefix)
+        .options(commandOptions);
     });
     this.commands[name] = executor;
     return this;
@@ -49,19 +55,30 @@ export class Program {
     // with the `global` flag so this makes sure every option has it.
     Object.keys(options).forEach((key) => {
       options[key].global = true;
+      if (options[key].demand === undefined) {
+        // By default, all options should be "demanded" otherwise
+        // yargs.strict() will think they are missing when declared.
+        options[key].demand = true;
+      }
     });
     this.yargs.options(options);
     return this;
   }
 
   run(absolutePackageDir: string,
-      {throwError=false, systemProcess=process, logStream=defaultLogStream,
-       getVersion=version}
+      {systemProcess=process, logStream=defaultLogStream,
+       getVersion=defaultVersionGetter, shouldExitProgram=true}
       : Object = {}): Promise {
-    let argv = this.yargs.argv;
-    let cmd = argv._[0];
+
+    this.shouldExitProgram = shouldExitProgram;
+    let argv;
+    let cmd;
+
     return new Promise(
       (resolve) => {
+        this.yargs.exitProcess(this.shouldExitProgram);
+        argv = this.yargs.argv;
+        cmd = argv._[0];
         if (cmd === undefined) {
           throw new WebExtError('No sub-command was specified in the args');
         }
@@ -83,17 +100,17 @@ export class Program {
           log.error(`${prefix}Error code: ${error.code}\n`);
         }
 
-        if (throwError) {
-          throw error;
-        } else {
+        if (this.shouldExitProgram) {
           systemProcess.exit(1);
+        } else {
+          throw error;
         }
       });
   }
 }
 
 
-export function version(absolutePackageDir: string): string {
+export function defaultVersionGetter(absolutePackageDir: string): string {
   let packageData: any = readFileSync(
     path.join(absolutePackageDir, 'package.json'));
   return JSON.parse(packageData).version;
@@ -102,7 +119,8 @@ export function version(absolutePackageDir: string): string {
 
 export function main(
     absolutePackageDir: string,
-    {commands=defaultCommands, argv, runOptions={}}: Object = {}): Promise {
+    {getVersion=defaultVersionGetter, commands=defaultCommands, argv,
+     runOptions={}}: Object = {}): Promise {
   let program = new Program(argv);
   // yargs uses magic camel case expansion to expose options on the
   // final argv object. For example, the 'artifacts-dir' option is alternatively
@@ -120,7 +138,7 @@ Example: $0 --help run.
     .help('help')
     .alias('h', 'help')
     .env(envPrefix)
-    .version(() => version(absolutePackageDir))
+    .version(() => getVersion(absolutePackageDir))
     .demand(1)
     .strict();
 
@@ -130,15 +148,13 @@ Example: $0 --help run.
       describe: 'Web extension source directory.',
       default: process.cwd(),
       requiresArg: true,
-      demand: true,
       type: 'string',
     },
     'artifacts-dir': {
       alias: 'a',
       describe: 'Directory where artifacts will be saved.',
       default: path.join(process.cwd(), 'web-ext-artifacts'),
       requiresArg: true,
-      demand: true,
       type: 'string',
     },
     'verbose': {
@@ -149,38 +165,40 @@ Example: $0 --help run.
   });
 
   program
-    .command('build',
-             'Create a web extension package from source',
-             commands.build, {
-      'as-needed': {
-        describe: 'Watch for file changes and re-build as needed',
-        type: 'boolean',
-      },
-    })
-    .command('sign',
-             'Sign the web extension so it can be installed in Firefox',
-             commands.sign, {
-      'api-key': {
-        describe: 'API key (JWT issuer) from addons.mozilla.org',
-        demand: true,
-        type: 'string',
-      },
-      'api-secret': {
-        describe: 'API secret (JWT secret) from addons.mozilla.org',
-        demand: true,
-        type: 'string',
-      },
-      'api-url-prefix': {
-        describe: 'Signing API URL prefix',
-        default: 'https://addons.mozilla.org/api/v3',
-        demand: true,
-        type: 'string',
-      },
-      'timeout' : {
-        describe: 'Number of milliseconds to wait before giving up',
-        type: 'number',
-      },
-    })
+    .command(
+      'build',
+      'Create a web extension package from source',
+      commands.build, {
+        'as-needed': {
+          describe: 'Watch for file changes and re-build as needed',
+          type: 'boolean',
+        },
+      })
+    .command(
+      'sign',
+      'Sign the web extension so it can be installed in Firefox',
+      commands.sign, {
+        'api-key': {
+          describe: 'API key (JWT issuer) from addons.mozilla.org',
+          demand: true,
+          type: 'string',
+        },
+        'api-secret': {
+          describe: 'API secret (JWT secret) from addons.mozilla.org',
+          demand: true,
+          type: 'string',
+        },
+        'api-url-prefix': {
+          describe: 'Signing API URL prefix',
+          default: 'https://addons.mozilla.org/api/v3',
+          demand: true,
+          type: 'string',
+        },
+        'timeout' : {
+          describe: 'Number of milliseconds to wait before giving up',
+          type: 'number',
+        },
+      })
     .command('run', 'Run the web extension', commands.run, {
       'firefox-binary': {
         describe: 'Path to a Firefox executable such as firefox-bin. ' +