Repository: yargs/yargs
Pull request #428 (2016-03-11 05:50:26)

Introduce command execution context
This is something I've wanted to do for a while but never had a good grasp on how to implement it. (And, I arguably still don't. :grinning:)

However, issue #426 brought up a good first use-case for maintaining a context of command execution, in order to change the behavior of "unknown argument" validation (induced by `.strict()`) based on if it's running for a (possibly nested) command or not.

As is, this branch fixes that problem - without breaking any existing tests - but is still lacking tests for the scenarios it addresses.

The reason I'm opening this PR now is that I'd like some feedback on the approach/implementation before I put much more effort into it. Basically, I'm adding another piece of state (tied to the yargs singleton) that does **not** get reset for command execution, but rather keeps track of which commands are currently being executed. This `context` object is available to all submodule logic (via the `.getContext()` method), intended to be **modified** at state transitions and **read** for processing logic that is dependent on the current command context. I realize that exposing it in this way leaves it vulnerable to unexpected manipulation (even from user code), but it seems more useful/flexible this way and is consistent with other "internal" APIs as well.

Note that the `context` initially has two fields: a `commands` array representing the currently executing commands (supports nested subcommands) and a `resets` integer representing the total number of times a reset has occurred. I thought the number of resets would be useful, but I ended up not using it since, for command processing, I can rely on the length of the commands array instead. I fully expect additional fields to be added to the `context` in the future, if the need arises for additional state not currently represented.

Also note that this branch already uses the `context` in two different ways - once for validation logic and once for correcting the implicit usage string for subcommands. I also commented a way it might be useful for interpreting `.demand()` for a command, but I shied away from this b/c I honestly don't understand the intent/expectation of demanding a certain number of args within a (sub)command.

At any rate, please let me know your thoughts. I pushed the branch to the main repo so it is available for easy fetching/modification by yargs members.

maxrimue: I really like the idea of this PR and the code looks good so far too :+1:

What's your idea on how `.demand()` could be used with the addition of context?
nexdrew: @maxrimue Well, when someone does `.demand(1)` from within a command builder function (which I've seen a few times in issues), are they trying to demand one _total_ flagless args or one _additional_ flagless arg? I mean, the command that the builder belongs to will not be processed/executed if at least one flagless arg was not already given (representing the command itself).

So what I'm speculating is that they really mean "when executing this command, require one additional flagless arg", which becomes a context-dependent interpretation of `.demand()`, such that we should probably _increment_ the total number of flagless args demanded by the value specified (e.g. 1 to represent the command and 1 more to represent the arg needed by the command handler) rather than _set_ the total number of flagless args demanded to the value specified. This idea is currently represented by the commented code here: https://github.com/yargs/yargs/blob/command-context/index.js#L254

Otherwise, it doesn't really make sense to `.demand(1)` inside a command builder, because we already know one flagless arg was given.
nexdrew: Concerning `.demand(1)` within a command builder, perhaps it's just unclear to users that the builder function is not executed until the command is given on the command line?

For an example of my confusion, see issue #353 (which was based on the 3.x API).

Either way, I think there's a mismatch between user expectations and yargs interpretation of command context.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/5477258/badge)](https://coveralls.io/builds/5477258)

Coverage remained the same at 100.0% when pulling **9e3b449c0d2a8f324bf76371ff4bee51544ab3a9 on command-context** into **95a0d535c4d5ca70fca1fc21e67baec0f135868f on master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/5485312/badge)](https://coveralls.io/builds/5485312)

Coverage remained the same at 100.0% when pulling **9c41d4e45d4df37579b752144cc23a2b081af934 on command-context** into **8abb75ada891189e3644dc0d63fe902b4aa2e846 on master**.

nexdrew: Now with tests for behavior that was modified to be context-aware! This PR represents initial support for subcommands (commands within commands) in yargs, which includes the following 3 areas:

1. Positional arg population now supports subcommands
2. Strict mode (unknown argument validation) now supports subcommands
3. Default usage text now supports subcommands

There may be other areas not yet addressed (e.g. should `.demand()` be contextual per command?), but these were the major ones I found in testing, and I think this is a good first step.

PR should be ready for final review.
maxrimue: What does everybody think about whether or not to include context for `.demand()`?

I think including context for `.demand()` makes sense and is probably what people would expect when demanding inside a command.
divmain: @maxrimue, I've been following this pull request because I ran into the same questions.  I'd definitely be in favor of contextual `.demand()`.  As a user, it is the behavior that I would expect.  Thanks for the work on this @nexdrew!
bcoe: @maxrimue I don't object to this one bit, but I think this should be a separate well thought out pull request. This pull already introduces quite a bit of new logic, and I'd rather keep new demand logic separate.
bcoe: @maxrimue @divmain If there's not already an issue open on the subject, start one going and let's think about what changes to the demand API should be made.
divmain: Looks like this is covered in #353.

Patch:
diff --git a/lib/command.js b/lib/command.js
index b046a66..6e38b48 100644
--- a/lib/command.js
+++ b/lib/command.js
@@ -70,6 +70,9 @@ module.exports = function (yargs, usage, validation) {
     var argv = parsed.argv
     var commandHandler = handlers[command]
     var innerArgv = argv
+    var currentContext = yargs.getContext()
+    var parentCommands = currentContext.commands.slice()
+    currentContext.commands.push(command)
     if (commandHandler.builder && typeof commandHandler.builder === 'function') {
       // a function can be provided, which interacts which builds
       // up a yargs chain and returns it.
@@ -79,30 +82,31 @@ module.exports = function (yargs, usage, validation) {
       // original command string as usage() for consistent behavior with
       // options object below
       if (yargs.parsed === false && typeof yargs.getUsageInstance().getUsage() === 'undefined') {
-        yargs.usage('$0 ' + commandHandler.original)
+        yargs.usage('$0 ' + (parentCommands.length ? parentCommands.join(' ') + ' ' : '') + commandHandler.original)
       }
       innerArgv = innerArgv ? innerArgv.argv : argv
     } else if (commandHandler.builder && typeof commandHandler.builder === 'object') {
       // as a short hand, an object can instead be provided, specifying
       // the options that a command takes.
       innerArgv = yargs.reset(parsed.aliases)
-      innerArgv.usage('$0 ' + commandHandler.original)
+      innerArgv.usage('$0 ' + (parentCommands.length ? parentCommands.join(' ') + ' ' : '') + commandHandler.original)
       Object.keys(commandHandler.builder).forEach(function (key) {
         innerArgv.option(key, commandHandler.builder[key])
       })
       innerArgv = innerArgv.argv
     }
 
-    populatePositional(commandHandler, innerArgv)
+    populatePositional(commandHandler, innerArgv, currentContext)
 
     if (commandHandler.handler) {
       commandHandler.handler(innerArgv)
     }
+    currentContext.commands.pop()
     return innerArgv
   }
 
-  function populatePositional (commandHandler, argv) {
-    var cmd = argv._.shift() // nuke the first argument (the current command)
+  function populatePositional (commandHandler, argv, context) {
+    argv._ = argv._.slice(context.commands.length) // nuke the current commands
     var demanded = commandHandler.demanded.slice(0)
     var optional = commandHandler.optional.slice(0)
 
@@ -120,7 +124,7 @@ module.exports = function (yargs, usage, validation) {
       argv[maybe] = argv._.shift()
     }
 
-    argv._.unshift(cmd)
+    argv._ = context.commands.concat(argv._)
   }
 
   self.reset = function () {
diff --git a/lib/validation.js b/lib/validation.js
index 0f86fe1..388f1c4 100644
--- a/lib/validation.js
+++ b/lib/validation.js
@@ -109,6 +109,7 @@ module.exports = function (yargs, usage, y18n) {
     var demanded = yargs.getDemanded()
     var commandKeys = yargs.getCommandInstance().getCommands()
     var unknown = []
+    var currentContext = yargs.getContext()
 
     Object.keys(aliases).forEach(function (key) {
       aliases[key].forEach(function (alias) {
@@ -126,7 +127,7 @@ module.exports = function (yargs, usage, y18n) {
     })
 
     if (commandKeys.length > 0) {
-      argv._.forEach(function (key) {
+      argv._.slice(currentContext.commands.length).forEach(function (key) {
         if (commandKeys.indexOf(key) === -1) {
           unknown.push(key)
         }
diff --git a/yargs.js b/yargs.js
index 9a10c72..1a130e8 100644
--- a/yargs.js
+++ b/yargs.js
@@ -47,11 +47,19 @@ function Yargs (processArgs, cwd, parentRequire) {
     )
   }
 
+  // use context object to keep track of resets, subcommand execution, etc
+  // submodules should modify and check the state of context as necessary
+  var context = { resets: -1, commands: [] }
+  self.getContext = function () {
+    return context
+  }
+
   // puts yargs back into an initial state. any keys
   // that have been set to "global" will not be reset
   // by this action.
   var options
   self.resetOptions = self.reset = function (aliases) {
+    context.resets++
     aliases = aliases || {}
     options = options || {}
     // put yargs back into an initial state, this


Test patch:
diff --git a/test/command.js b/test/command.js
index 4e895d5..ec08c32 100644
--- a/test/command.js
+++ b/test/command.js
@@ -41,6 +41,23 @@ describe('Command', function () {
       argv.bar.should.equal('hello')
       argv.awesome.should.equal('world')
     })
+
+    it('populates subcommand\'s inner argv with positional arguments', function () {
+      yargs('foo bar hello world')
+        .command('foo', 'my awesome command', function (yargs) {
+          return yargs.command(
+            'bar <greeting> [recipient]',
+            'subcommands are cool',
+            function () {},
+            function (argv) {
+              argv._.should.deep.equal(['foo', 'bar'])
+              argv.greeting.should.equal('hello')
+              argv.recipient.should.equal('world')
+            }
+          )
+        })
+        .argv
+    })
   })
 
   describe('missing positional arguments', function () {
diff --git a/test/usage.js b/test/usage.js
index 0bb17b2..c322e59 100644
--- a/test/usage.js
+++ b/test/usage.js
@@ -1717,6 +1717,44 @@ describe('usage tests', function () {
         ''
       ])
     })
+
+    it('displays given command chain with positional args in default usage for subcommand with builder object', function () {
+      var r = checkUsage(function () {
+        return yargs('one two --help')
+          .command('one <sub>', 'level one, requires subcommand', function (yargs) {
+            return yargs.command('two [next]', 'level two', {}, function (argv) {})
+          }, function (argv) {})
+          .help().wrap(null)
+          .argv
+      })
+
+      r.logs[0].split('\n').should.deep.equal([
+        './usage one two [next]',
+        '',
+        'Options:',
+        '  --help  Show help  [boolean]',
+        ''
+      ])
+    })
+
+    it('displays given command chain with positional args in default usage for subcommand with builder function', function () {
+      var r = checkUsage(function () {
+        return yargs('one two --help')
+          .command('one <sub>', 'level one, requires subcommand', function (yargs) {
+            return yargs.command('two [next]', 'level two', function (yargs) { return yargs }, function (argv) {})
+          }, function (argv) {})
+          .help().wrap(null)
+          .argv
+      })
+
+      r.logs[0].split('\n').should.deep.equal([
+        './usage one two [next]',
+        '',
+        'Options:',
+        '  --help  Show help  [boolean]',
+        ''
+      ])
+    })
   })
 
   describe('epilogue', function () {
diff --git a/test/validation.js b/test/validation.js
index a54d0e5..70ba7fc 100644
--- a/test/validation.js
+++ b/test/validation.js
@@ -315,4 +315,22 @@ describe('validation tests', function () {
         .parse([])
     })
   })
+
+  describe('strict mode', function () {
+    it('does not fail when command with subcommands called', function () {
+      yargs('one')
+        .command('one', 'level one', function (yargs) {
+          return yargs
+            .command('twoA', 'level two A')
+            .command('twoB', 'level two B')
+            .strict()
+            .fail(function (msg) {
+              expect.fail()
+            })
+        }, function (argv) {
+          argv._[0].should.equal('one')
+        })
+        .argv
+    })
+  })
 })
diff --git a/test/yargs.js b/test/yargs.js
index 93c8b0f..f8494d9 100644
--- a/test/yargs.js
+++ b/test/yargs.js
@@ -887,3 +887,43 @@ describe('yargs dsl tests', function () {
     })
   })
 })
+
+describe('yargs context', function () {
+  beforeEach(function () {
+    delete require.cache[require.resolve('../')]
+    yargs = require('../')
+  })
+
+  it('should begin with initial state', function () {
+    var context = yargs.getContext()
+    context.resets.should.equal(0)
+    context.commands.should.deep.equal([])
+  })
+
+  it('should track number of resets', function () {
+    var context = yargs.getContext()
+    yargs.reset()
+    context.resets.should.equal(1)
+    yargs.reset()
+    yargs.reset()
+    context.resets.should.equal(3)
+  })
+
+  it('should track commands being executed', function () {
+    var context
+    yargs('one two')
+      .command('one', 'level one', function (yargs) {
+        context = yargs.getContext()
+        context.commands.should.deep.equal(['one'])
+        return yargs.command('two', 'level two', function (yargs) {
+          context.commands.should.deep.equal(['one', 'two'])
+        }, function (argv) {
+          context.commands.should.deep.equal(['one', 'two'])
+        })
+      }, function (argv) {
+        context.commands.should.deep.equal(['one'])
+      })
+      .argv
+    context.commands.should.deep.equal([])
+  })
+})