Repository: yargs/yargs (js)

Issue:
Unknown arguments cause incorrect command positional `coerce()`
As evidenced in https://github.com/lerna/lerna/issues/2315, when arguments are passed after `--` with the default `parserConfiguration` (append them to `argv._`), `yargs@14.2.0` causes an incorrect `coerce("")` call on a command's positional argument, as well as re-parsing any flags (which fails when `strict()` is enabled).

```js
require('yargs')
  .strict()
  .option('loglevel', {
    type: 'string',
  })
  .command('repro [stuff]', 'example', yargs => {
    yargs.positional('stuff', {
      describe: 'things',
      coerce: stuff => {
        if (stuff === '') {
          throw new Error('fail');
        }
        return stuff;
      },
    });
  }, argv => {
    console.log(argv);
  })
  .parse('repro -- --loglevel=woo');
```

It _also_ causes the valid `--loglevel` option in the example above to fail, once the `coerce()` method is modfied to ensure that the parameter is in fact non-empty first (or if it's an identity function `stuff => stuff`, or removed entirely). So that's fun.

The current workaround is to enable `populate--` in yargs-parser:
```js
  .parserConfiguration({
    "populate--": true,
  })
```

This seems sub-optimal.

Pull request description:
fix: -- (stop parse) was not being respected by commands
@evocateur I believe this addresses at least half of the issues you described in #1457 (it sounded like you were potentially describing two bugs).

mind trying the branch out?

fixes #1457 

Patch:
diff --git a/lib/command.js b/lib/command.js
index d8c0e80..6f6dd0d 100644
--- a/lib/command.js
+++ b/lib/command.js
@@ -227,9 +227,12 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) {
 
     if (commandHandler.handler && !yargs._hasOutput()) {
       yargs._setHasOutput()
+      // to simplify the parsing of positionals in commands,
+      // we temporarily populate '--' rather than _, with arguments
+      const populateDoubleDash = !!yargs.getOptions().configuration['populate--']
+      if (!populateDoubleDash) yargs._copyDoubleDash(innerArgv)
 
       innerArgv = applyMiddleware(innerArgv, yargs, middlewares, false)
-
       let handlerResult
       if (isPromise(innerArgv)) {
         handlerResult = innerArgv.then(argv => commandHandler.handler(argv))
@@ -349,7 +352,12 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) {
     // short-circuit parse.
     if (!unparsed.length) return
 
-    const parsed = Parser.detailed(unparsed, options)
+    const config = Object.assign({}, options.configuration, {
+      'populate--': true
+    })
+    const parsed = Parser.detailed(unparsed, Object.assign({}, options, {
+      configuration: config
+    }))
 
     if (parsed.error) {
       yargs.getUsageInstance().fail(parsed.error.message, parsed.error)
diff --git a/package.json b/package.json
index 475f4b2..c7d4bae 100644
--- a/package.json
+++ b/package.json
@@ -49,7 +49,7 @@
   },
   "scripts": {
     "fix": "standard --fix",
-    "pretest": "standard",
+    "posttest": "standard",
     "test": "c8 --reporter=html --reporter=text mocha --require ./test/before.js --timeout=12000 --check-leaks",
     "coverage": "c8 report --reporter=text-lcov | coveralls",
     "release": "standard-version"
diff --git a/yargs.js b/yargs.js
index 48658f3..24db759 100644
--- a/yargs.js
+++ b/yargs.js
@@ -1030,14 +1030,21 @@ function Yargs (processArgs, cwd, parentRequire) {
     enumerable: true
   })
 
-  self._parseArgs = function parseArgs (args, shortCircuit, _skipValidation, commandIndex) {
-    let skipValidation = !!_skipValidation
+  self._parseArgs = function parseArgs (args, shortCircuit, _calledFromCommand, commandIndex) {
+    let skipValidation = !!_calledFromCommand
     args = args || processArgs
 
     options.__ = y18n.__
     options.configuration = self.getParserConfiguration()
 
-    const parsed = Parser.detailed(args, options)
+    const populateDoubleDash = !!options.configuration['populate--']
+    const config = Object.assign({}, options.configuration, {
+      'populate--': true
+    })
+    const parsed = Parser.detailed(args, Object.assign({}, options, {
+      configuration: config
+    }))
+
     let argv = parsed.argv
     if (parseContext) argv = Object.assign({}, argv, parseContext)
     const aliases = parsed.aliases
@@ -1052,7 +1059,7 @@ function Yargs (processArgs, cwd, parentRequire) {
       // are two passes through the parser. If completion
       // is being performed short-circuit on the first pass.
       if (shortCircuit) {
-        return argv
+        return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
       }
 
       // if there's a handler associated with a
@@ -1085,7 +1092,8 @@ function Yargs (processArgs, cwd, parentRequire) {
               // commands are executed using a recursive algorithm that executes
               // the deepest command first; we keep track of the position in the
               // argv._ array that is currently being executed.
-              return command.runCommand(cmd, self, parsed, i + 1)
+              const innerArgv = command.runCommand(cmd, self, parsed, i + 1)
+              return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
             } else if (!firstUnknownCommand && cmd !== completionCommand) {
               firstUnknownCommand = cmd
               break
@@ -1094,7 +1102,8 @@ function Yargs (processArgs, cwd, parentRequire) {
 
           // run the default command, if defined
           if (command.hasDefaultCommand() && !skipDefaultCommand) {
-            return command.runCommand(null, self, parsed)
+            const innerArgv = command.runCommand(null, self, parsed)
+            return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
           }
 
           // recommend a command if recommendCommands() has
@@ -1111,7 +1120,8 @@ function Yargs (processArgs, cwd, parentRequire) {
           self.exit(0)
         }
       } else if (command.hasDefaultCommand() && !skipDefaultCommand) {
-        return command.runCommand(null, self, parsed)
+        const innerArgv = command.runCommand(null, self, parsed)
+        return populateDoubleDash ? innerArgv : self._copyDoubleDash(innerArgv)
       }
 
       // we must run completions first, a user might
@@ -1129,7 +1139,7 @@ function Yargs (processArgs, cwd, parentRequire) {
 
           self.exit(0)
         })
-        return argv
+        return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
       }
 
       // Handle 'help' and 'version' options
@@ -1173,6 +1183,16 @@ function Yargs (processArgs, cwd, parentRequire) {
       else throw err
     }
 
+    return (populateDoubleDash || _calledFromCommand) ? argv : self._copyDoubleDash(argv)
+  }
+
+  // to simplify the parsing of positionals in commands,
+  // we temporarily populate '--' rather than _, with arguments
+  // after the '--' directive. After the parse, we copy these back.
+  self._copyDoubleDash = function (argv) {
+    if (!argv._) return argv
+    argv._.push.apply(argv._, argv['--'])
+    delete argv['--']
     return argv
   }
 


Test patch:
diff --git a/test/command.js b/test/command.js
index d3a5628..6a90f89 100644
--- a/test/command.js
+++ b/test/command.js
@@ -135,6 +135,45 @@ describe('Command', () => {
         .parse()
       parseCount.should.equal(1)
     })
+
+    // see: https://github.com/yargs/yargs/issues/1457
+    it('handles -- in conjunction with positional arguments', () => {
+      let called = false
+      const argv = yargs('foo hello world series -- apple banana')
+        .command('foo <bar> [awesome...]', 'my awesome command', noop, (argv2) => {
+          argv2.bar.should.eql('hello')
+          argv2.awesome.should.eql(['world', 'series'])
+          argv2['_'].should.eql(['foo', 'apple', 'banana'])
+          called = true
+        })
+        .parse()
+      argv.bar.should.eql('hello')
+      argv.awesome.should.eql(['world', 'series'])
+      argv['_'].should.eql(['foo', 'apple', 'banana'])
+      called.should.equal(true)
+    })
+
+    // see: https://github.com/yargs/yargs/issues/1457
+    it('continues to support populate-- for commands, post #1457', () => {
+      let called = false
+      const argv = yargs('foo hello world series -- apple banana')
+        .command('foo <bar> [awesome...]', 'my awesome command', noop, (argv2) => {
+          argv2.bar.should.eql('hello')
+          argv2.awesome.should.eql(['world', 'series'])
+          argv2['_'].should.eql(['foo'])
+          argv2['--'].should.eql(['apple', 'banana'])
+          called = true
+        })
+        .parserConfiguration({
+          'populate--': true
+        })
+        .parse()
+      argv.bar.should.eql('hello')
+      argv.awesome.should.eql(['world', 'series'])
+      argv['_'].should.eql(['foo'])
+      argv['--'].should.eql(['apple', 'banana'])
+      called.should.equal(true)
+    })
   })
 
   describe('variadic', () => {