fix: address positional argument strict() bug introduced in #766 (#784)

bcoe · bcoe · commit a8528e6a6cac · 2017-02-26T09:53:21.000-06:00
diff --git a/lib/command.js b/lib/command.js
@@ -140,6 +140,7 @@ module.exports = function (yargs, usage, validation) {
     // what does yargs look like after the buidler is run?
     var innerArgv = parsed.argv
     var innerYargs = null
+    var positionalMap = {}
 
     currentContext.commands.push(command)
     if (typeof commandHandler.builder === 'function') {
@@ -173,15 +174,17 @@ module.exports = function (yargs, usage, validation) {
       aliases = innerYargs.parsed.aliases
     }
 
-    if (!yargs._hasOutput()) populatePositionals(commandHandler, innerArgv, currentContext, yargs)
-
-    if (commandHandler.handler && !yargs._hasOutput()) {
-      commandHandler.handler(innerArgv)
+    if (!yargs._hasOutput()) {
+      positionalMap = populatePositionals(commandHandler, innerArgv, currentContext, yargs)
     }
 
     // we apply validation post-hoc, so that custom
     // checks get passed populated positional arguments.
-    yargs._runValidation(innerArgv, aliases)
+    yargs._runValidation(innerArgv, aliases, positionalMap)
+
+    if (commandHandler.handler && !yargs._hasOutput()) {
+      commandHandler.handler(innerArgv)
+    }
 
     currentContext.commands.pop()
     numFiles = currentContext.files.length - numFiles
@@ -196,25 +199,27 @@ module.exports = function (yargs, usage, validation) {
     argv._ = argv._.slice(context.commands.length) // nuke the current commands
     var demanded = commandHandler.demanded.slice(0)
     var optional = commandHandler.optional.slice(0)
+    var positionalMap = {}
 
     validation.positionalCount(demanded.length, argv._.length)
 
     while (demanded.length) {
       var demand = demanded.shift()
-      populatePositional(demand, argv, yargs)
+      populatePositional(demand, argv, yargs, positionalMap)
     }
 
     while (optional.length) {
       var maybe = optional.shift()
-      populatePositional(maybe, argv, yargs)
+      populatePositional(maybe, argv, yargs, positionalMap)
     }
 
     argv._ = context.commands.concat(argv._)
+    return positionalMap
   }
 
   // populate a single positional argument and its
   // aliases onto argv.
-  function populatePositional (positional, argv, yargs) {
+  function populatePositional (positional, argv, yargs, positionalMap) {
     // "positional" consists of the positional.cmd, an array representing
     // the positional's name and aliases, and positional.variadic
     // indicating whether or not it is a variadic array.
@@ -229,6 +234,7 @@ module.exports = function (yargs, usage, validation) {
         if (value) argv[cmd] = value
         else argv[cmd] = value = argv._.shift()
       }
+      positionalMap[cmd] = true
       postProcessPositional(yargs, argv, cmd)
       addCamelCaseExpansions(argv, cmd)
     }
diff --git a/lib/validation.js b/lib/validation.js
@@ -116,7 +116,7 @@ module.exports = function (yargs, usage, y18n) {
   }
 
   // check for unknown arguments (strict-mode).
-  self.unknownArguments = function (argv, aliases) {
+  self.unknownArguments = function (argv, aliases, positionalMap) {
     const aliasLookup = {}
     const descriptions = usage.getDescriptions()
     const demandedOptions = yargs.getDemandedOptions()
@@ -134,6 +134,7 @@ module.exports = function (yargs, usage, y18n) {
       if (key !== '$0' && key !== '_' &&
         !descriptions.hasOwnProperty(key) &&
         !demandedOptions.hasOwnProperty(key) &&
+        !positionalMap.hasOwnProperty(key) &&
         !aliasLookup.hasOwnProperty(key)) {
         unknown.push(key)
       }
diff --git a/test/command.js b/test/command.js
@@ -986,6 +986,31 @@ describe('Command', function () {
         yargs.argv // parse and run command
         commandCalled.should.be.true
       })
+
+      // address regression introduced in #766, thanks @nexdrew!
+      it('does not fail strict check due to postional command arguments', function (done) {
+        yargs()
+          .strict()
+          .command('hi <name>', 'The hi command')
+          .parse('hi ben', function (err, argv, output) {
+            expect(err).to.equal(null)
+            return done()
+          })
+      })
+
+      it('does not fire command if validation fails', function (done) {
+        var commandRun = false
+        yargs()
+          .strict()
+          .command('hi <name>', 'The hi command', function () {}, function (argv) {
+            commandRun = true
+          })
+          .parse('hi ben --hello=world', function (err, argv, output) {
+            commandRun.should.equal(false)
+            err.message.should.equal('Unknown argument: hello')
+            return done()
+          })
+      })
     })
 
     describe('types', function () {
diff --git a/yargs.js b/yargs.js
@@ -1051,18 +1051,18 @@ function Yargs (processArgs, cwd, parentRequire) {
       // if we're executed via bash completion, don't
       // bother with validation.
       if (!argv[completion.completionKey]) {
-        self._runValidation(argv, aliases)
+        self._runValidation(argv, aliases, {})
       }
     }
 
     return setPlaceholderKeys(argv)
   }
 
-  self._runValidation = function (argv, aliases) {
+  self._runValidation = function (argv, aliases, positionalMap) {
     validation.nonOptionCount(argv)
     validation.missingArgumentValue(argv)
     validation.requiredArguments(argv)
-    if (strict) validation.unknownArguments(argv, aliases)
+    if (strict) validation.unknownArguments(argv, aliases, positionalMap)
     validation.customChecks(argv, aliases)
     validation.limitedChoices(argv)
     validation.implications(argv)
