Repository: yargs/yargs
Pull request #1256 (2018-12-11 21:16:24)

Adds sort to command usage help output
When showing the usage (if no command is available) the order of commands isn't sorted, this might be a undesirable and could be put into a config setting, but I'd like to see the tolerance to default to sorted rather than as-defined.

bcoe: @trevorlinton the nice thing about not applying sorting, is folks can control the ordering of commands by definition order.

my concern with this change is it would break a lot of downstream consumers of yargs, and would need to be a breaking change.
trevorlinton: can I add it as an optional config (disabled by default?) the particular issue I'm under is I don't have control with the timing when new commands are registered, so the ordering can at times be random.
bcoe: @trevorlinton that's an interesting requirement; I'd be open to this, I've been chatting with @cspotcode about adding the ability to configure compiler settings with a flag like `.yargsConfig` which would pass config to the `yargs-parser`; what if we allowed certain yargs settings in the non-parser layer to be configured with this too, e.g., `sort-commands` which defaults to `false`.

FYI, I think you can use `String.localeCompare()` to perform the sortinng.
bcoe: @trevorlinton see https://github.com/yargs/yargs/pull/1262, I'm comfortable with `sort-commands` being a configuration option that can be passed to yargs via `parserConfiguration`.
trevorlinton: @evocateur @bcoe I added in the `sort-commands` based on the work done in #1262 is there a common branch I can do a PR/rebase into on yargs? otherwise I'm going to be pulling in all the commits/changes from #1262 and our merges become very similar (or, alternatively, merge #1262 first and leave this be until then).
bcoe: @trevorlinton I'll get #1262 landed ASAP, and then you can rebase against `master` 👍 
bcoe: @trevorlinton thanks for your patience, if you don't mind rebasing I'll give this a final review 👍

one note, we'll want to make sure we document the new API method appropriately.
trevorlinton: @bcoe I've rebased on master, you can review the file changes, it may be prudent to squash this merge considering all of the conflicts/rebasing that occurred by merging in a unmerged PR and diverging. I've added some notes in api.md on the config option, but I'm not sure how we should document this properly, I'm hoping what's there is good enough or now.

Patch:
diff --git a/docs/api.md b/docs/api.md
index d20aa0e..e229d56 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -1151,8 +1151,13 @@ for details of this object
 
 <a name="parserConfiguration"></a>.parserConfiguration(obj)
 ------------
-`parserConfiguration()` allows you to configure yargs. See [yargs-parser's configuration](https://github.com/yargs/yargs-parser#configuration) for
-settings specific to `yargs-parser`.
+`parserConfiguration()` allows you to configure advanced yargs features.
+
+`obj` accepts the following configuration options:
+
+* `sort-commands` when set to `true` (boolean) will sort the commands added, the default is `false`.
+
+For additional configuration options, see [yargs-parser's configuration](https://github.com/yargs/yargs-parser#configuration).
 
 <a name="pkg-conf"></a>
 .pkgConf(key, [cwd])
diff --git a/lib/command.js b/lib/command.js
index fdf9e37..e643d16 100644
--- a/lib/command.js
+++ b/lib/command.js
@@ -17,12 +17,11 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) {
   let aliasMap = {}
   let defaultCommand
   globalMiddleware = globalMiddleware || []
-  self.addHandler = function addHandler (cmd, description, builder, handler, middlewares) {
+  self.addHandler = function addHandler (cmd, description, builder, handler, _middlewares) {
     let aliases = []
+    const middlewares = (_middlewares || []).slice(0)
     handler = handler || (() => {})
-    middlewares = middlewares || []
-    globalMiddleware.push(...middlewares)
-    middlewares = globalMiddleware
+
     if (Array.isArray(cmd)) {
       aliases = cmd.slice(1)
       cmd = cmd[0]
@@ -232,7 +231,7 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) {
     if (commandHandler.handler && !yargs._hasOutput()) {
       yargs._setHasOutput()
 
-      const middlewares = commandHandler.middlewares || []
+      const middlewares = globalMiddleware.slice(0).concat(commandHandler.middlewares || [])
 
       innerArgv = applyMiddleware(innerArgv, middlewares)
 
diff --git a/lib/usage.js b/lib/usage.js
index 9be8a99..f6daeed 100644
--- a/lib/usage.js
+++ b/lib/usage.js
@@ -204,6 +204,10 @@ module.exports = function usage (yargs, y18n) {
       const context = yargs.getContext()
       const parentCommands = context.commands.length ? `${context.commands.join(' ')} ` : ''
 
+      if (yargs.getParserConfiguration()['sort-commands'] === true) {
+        commands = commands.sort((a, b) => a[0].localeCompare(b[0]))
+      }
+
       commands.forEach((command) => {
         const commandString = `${base$0} ${parentCommands}${command[0].replace(/^\$0 ?/, '')}` // drop $0 from default commands.
         ui.span(
diff --git a/package.json b/package.json
index 31b549f..0945d1f 100644
--- a/package.json
+++ b/package.json
@@ -49,7 +49,7 @@
   },
   "scripts": {
     "pretest": "standard",
-    "test": "nyc --cache mocha --require ./test/before.js --timeout=8000 --check-leaks",
+    "test": "nyc --cache mocha --require ./test/before.js --timeout=12000 --check-leaks",
     "coverage": "nyc report --reporter=text-lcov | coveralls",
     "release": "standard-version"
   },


Test patch:
diff --git a/test/middleware.js b/test/middleware.js
index 0dfae92..a19a9fd 100644
--- a/test/middleware.js
+++ b/test/middleware.js
@@ -140,4 +140,25 @@ describe('middleware', () => {
         .parse()
     })
   })
+
+  // see: https://github.com/yargs/yargs/issues/1281
+  it("doesn't modify globalMiddleware array when executing middleware", () => {
+    let count = 0
+    yargs('bar')
+      .middleware((argv) => {
+        count++
+      })
+      .command('foo', 'foo command', () => {}, () => {}, [
+        () => {
+          count++
+        }
+      ])
+      .command('bar', 'bar command', () => {}, () => {}, [
+        () => {
+          count++
+        }
+      ])
+      .parse()
+    count.should.equal(2)
+  })
 })
diff --git a/test/usage.js b/test/usage.js
index cfa828f..022825c 100644
--- a/test/usage.js
+++ b/test/usage.js
@@ -2599,6 +2599,32 @@ describe('usage tests', () => {
       ])
     })
 
+    it('should display top-level help with sorting with no command given if sorting enabled', () => {
+      const r = checkUsage(() => yargs('--help')
+        .command(['list [pattern]', 'ls', '*'], 'List key-value pairs for pattern', {}, noop)
+        .command('get <key>', 'Get value for key', {}, noop)
+        .command('set <key> [value]', 'Set value for key', {}, noop)
+        .parserConfiguration({'sort-commands': true})
+        .parse()
+      )
+
+      r.logs[0].split('\n').should.deep.equal([
+        'usage [pattern]',
+        '',
+        'List key-value pairs for pattern',
+        '',
+        'Commands:',
+        '  usage get <key>          Get value for key',
+        '  usage list [pattern]     List key-value pairs for pattern',
+        '                                                         [default] [aliases: ls]',
+        '  usage set <key> [value]  Set value for key',
+        '',
+        'Options:',
+        '  --help     Show help                                                 [boolean]',
+        '  --version  Show version number                                       [boolean]'
+      ])
+    })
+
     it('should display default command as ./$0 if it has no aliases', () => {
       const r = checkUsage(() => yargs('--help')
         .command('* [pattern]', 'List key-value pairs for pattern', {}, noop)