Add .commandDir(dir) to API to apply all command modules from a relative directory #494

nexdrew · 2016-05-03T15:44:24Z

This replaces #463, opting to require modules from a single directory at a time via require-directory (instead of supporting globs).

The implementation is slightly trickier than I originally thought, but it allows a consumer to do things like the following.

Example setup

directory structure:

cli.js
commands/
  people.js
  places.js
  things.js
  common_cmds/
    details.js
    search.js
  people_cmds/
    call.js
  places_cmds/
    nav.js

cli.js:

#!/usr/bin/env node
require('yargs')
  // apply people.js, places.js, things.js but not subdirectories
  .commandDir('commands')
  .help()
  .wrap(40)
  .argv

commands/people.js:

exports.command = 'people <command>'
exports.desc = 'Work with people'
exports.builder = function (yargs) {
  // apply commands in subdirectories
  return yargs.commandDir('common_cmds').commandDir('people_cmds')
}
exports.handler = function (argv) {}

commands/places.js:

exports.command = 'places <command>'
exports.desc = 'Work with places'
exports.builder = function (yargs) {
  // you can even share the same subcommands e.g. common_cmds
  return yargs.commandDir('common_cmds').commandDir('places_cmds')
}
exports.handler = function (argv) {}

commands/things.js:

exports.command = 'things <command>'
exports.desc = 'Work with things'
exports.builder = function (yargs) {
  return yargs.commandDir('common_cmds')
}
exports.handler = function (argv) {}

commands/common_cmds/details.js:

exports.command = 'details <name>'
exports.desc = 'Get details by name'
exports.builder = {}
exports.handler = function (argv) {
  // parent command accessible via argv._ as always
  console.log('getting %s details for name: %s', argv._[0], argv.name)
}

Example CLI

$ ./cli.js --help
Commands:
  people <command>  Work with people
  places <command>  Work with places
  things <command>  Work with things

Options:
  --help  Show help            [boolean]

$ ./cli.js people --help
cli.js people <command>

Commands:
  details <name>  Get details by name
  search <query>  Search via query
  call <person>   Call someone

Options:
  --help  Show help            [boolean]

$ ./cli.js people details       
cli.js people details <name>

Options:
  --help  Show help            [boolean]

Not enough non-option arguments: got 0, need at least 1

$ ./cli.js people details Andrew
getting people details for name: Andrew

API

This adds one method to the API: .commandDir(dir, [opts])

dir is a directory name which can/should be relative to the module calling the method.

opts is an options object that is passed to require-directory, allowing consumers to customize the behavior. In the initial implementation, the following opts are valid:

opts.recurse: boolean, default false

Should yargs require command modules from all nested directories? The default is false so subcommands can be nested instead of flattened.
opts.extensions: array of strings, default ['js']

The types of files to include when requiring.
opts.visit: function

A synchronous function that is called for each command module encountered. Accepts commandObject, pathToFile, and filename as arguments. Returns commandObject to include the command; any falsey value to exclude/skip it.
opts.include: RegExp or function

Whitelist certain modules. See require-directory whitelisting for details.
opts.exclude: RegExp or function

Blacklist certain modules. See require-directory blacklisting for details.

Notes

This also includes a change that allows a command module to only define command and description, which better supports requiring JSON modules (though I'm not sure why someone would want to do that) and fulfills (at least) a part of #492.

I considered adding support for using the filename of a command module as its command string if the module itself doesn't define command, but I think it'd be nice to support this for all command modules (i.e. not just when using .commandDir()), so I'd like to tackle that separately/subsequently.

I don't have strong feelings about the method name commandDir. Would be happy to change it if folks like something else better. As always, feedback is welcomed!

Still lacking

Tests!
README update

coveralls · 2016-05-03T15:55:01Z

Coverage decreased (-1.9%) to 98.089% when pulling 8b2374d on command-dir into 82d4412 on master.

bcoe · 2016-05-14T18:39:15Z

@nexdrew I love this work, don't suppose we could get a test in the mix :)

AGresvig · 2016-05-19T20:11:24Z

Yes, this is exactly what I had hoped for - I think the chosen approach makes perfect sense and complements the functionality provided by command modules really well. Love it.
Please let this find it's way into master in the near future.

Great work @nexdrew

nexdrew · 2016-05-19T20:18:08Z

@AGresvig Thanks! I'll get some tests and docs written up within the next couple days so we can merge this safely.

coveralls · 2016-05-23T21:13:25Z

Coverage remained the same at 100.0% when pulling 1d566c8 on command-dir into a54c742 on master.

coveralls · 2016-05-24T22:18:01Z

Coverage remained the same at 100.0% when pulling 2b7d691 on command-dir into a54c742 on master.

nexdrew · 2016-05-25T22:27:14Z

@bcoe @maxrimue I believe this is ready for final review.

maxrimue · 2016-05-27T19:38:45Z

lib/command.js

+        self.addHandler(cmd.command, extractDesc(cmd), cmd.builder, cmd.handler)
+      } else {
+        self.addHandler(cmd.command, extractDesc(cmd))
+      }


I'm a little confused here, is allowing commands without builder and handler functions an essential feature for .commandDir() or is this unrelated?

@maxrimue Good question. This could certainly be done in a separate PR (maybe as a precursor) but is just something I ran into when testing with require-directory.

A couple reasons:

yargs currently blows up when you give it a command module/object that does not have all the expected properties of a command module - this is something that should probably be fixed in general

require-directory looks for and loads json files as well as js and coffee modules by default, which I have overridden to be just js (since a JSON "command module" cannot support a handler function by itself), but if folks want to define "commands" via JSON files, then they will need this change to support an option of extensions: ['js', 'json'] (or similar)

I don't really think no. 2 is a big concern, unless folks are dynamically generating their commands as JSON (not sure why you would), but is more of a "why not?".

I view no. 1 as a bigger problem that we should address, either before or with #492. So figured I'd kill two birds with one stone here. I can easily be persuaded otherwise.

Let's address this in #429 👍

maxrimue · 2016-05-27T19:41:46Z

Looks awesome, well done! Could we perhaps make some variables constants since there are some good reasons for doing so in yargs in general as shown in #502?

nexdrew · 2016-05-27T19:59:25Z

Could we perhaps make some variables constants since there are some good reasons for doing so in yargs in general as shown in #502?

@maxrimue Ooh, good catch! I think there are probably a couple places I could use const in here. I'll review that and push whatever changes are compatible with tests.

nexdrew · 2016-05-27T21:14:00Z

@maxrimue Swapped var for const in commit 0d14096.

maxrimue · 2016-05-27T21:24:36Z

@nexdrew Awesome! LGTM

coveralls · 2016-05-27T21:25:26Z

Coverage remained the same at 100.0% when pulling 0d14096 on command-dir into 5dd1e63 on master.

bcoe · 2016-05-30T01:28:58Z

package.json

@@ -19,6 +19,7 @@
    "os-locale": "^1.4.0",
    "pkg-conf": "^1.1.2",
    "read-pkg-up": "^1.0.1",
+    "require-directory": "^2.1.1",


I appreciate that this only pull in once dependency in total \o/

bcoe · 2016-07-09T21:41:43Z

@nexdrew please give npm cache clear; npm i yargs@next a spin I'm feeling ready to publish 4.8.0 once windows tests are passing.

bcoe · 2016-07-09T21:42:35Z

@AGresvig @maxrimue would love to also have you try out the next candidate release too 👍

npm cache clear; npm i yargs@next

AGresvig · 2016-07-11T07:15:24Z

@bcoe OK will do!

nexdrew mentioned this pull request May 3, 2016

[WIP] New method to apply command modules matching glob pattern #463

Closed

nexdrew mentioned this pull request May 19, 2016

multiple commands #225

Closed

nexdrew force-pushed the command-dir branch from 8b2374d to 1d566c8 Compare May 23, 2016 20:48

nexdrew changed the title ~~[WIP] Add .commandDir(dir) to API to apply all command modules from a relative directory~~ Add .commandDir(dir) to API to apply all command modules from a relative directory May 24, 2016

maxrimue reviewed May 27, 2016
View reviewed changes

nexdrew added 5 commits May 27, 2016 16:10

feat: add .commandDir() API using require-directory

81caf02

support for nested subcommands with relative dir

e81136e

allow command module to only define command and desc

3c07bd4

test: add some for commandDir

cf6fd2f

docs: document the .commandDir() method

63c6eed

nexdrew mentioned this pull request May 27, 2016

Command aliases #121

Closed

use const where possible based on @maxrimue's review

0d14096

nexdrew force-pushed the command-dir branch from 2b7d691 to 0d14096 Compare May 27, 2016 21:11

bcoe reviewed May 30, 2016
View reviewed changes

bcoe merged commit b299dff into master May 30, 2016

nexdrew deleted the command-dir branch May 30, 2016 02:19

nexdrew mentioned this pull request Jun 6, 2016

feat(command): derive missing command string from module filename #527

Merged

nexdrew mentioned this pull request Mar 15, 2020

Sub-commands mattallty/Caporal.js#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .commandDir(dir) to API to apply all command modules from a relative directory #494

Add .commandDir(dir) to API to apply all command modules from a relative directory #494

nexdrew commented May 3, 2016 •

edited

coveralls commented May 3, 2016 •

edited

bcoe commented May 14, 2016

AGresvig commented May 19, 2016

nexdrew commented May 19, 2016

coveralls commented May 23, 2016 •

edited

coveralls commented May 24, 2016 •

edited

nexdrew commented May 25, 2016

maxrimue May 27, 2016

nexdrew May 27, 2016

bcoe May 30, 2016

maxrimue commented May 27, 2016

nexdrew commented May 27, 2016

nexdrew commented May 27, 2016

maxrimue commented May 27, 2016

coveralls commented May 27, 2016 •

edited

bcoe May 30, 2016

bcoe commented Jul 9, 2016

bcoe commented Jul 9, 2016

AGresvig commented Jul 11, 2016

Add .commandDir(dir) to API to apply all command modules from a relative directory #494

Add .commandDir(dir) to API to apply all command modules from a relative directory #494

Conversation

nexdrew commented May 3, 2016 • edited

Example setup

Example CLI

API

Notes

Still lacking

coveralls commented May 3, 2016 • edited

bcoe commented May 14, 2016

AGresvig commented May 19, 2016

nexdrew commented May 19, 2016

coveralls commented May 23, 2016 • edited

coveralls commented May 24, 2016 • edited

nexdrew commented May 25, 2016

maxrimue May 27, 2016

Choose a reason for hiding this comment

nexdrew May 27, 2016

Choose a reason for hiding this comment

bcoe May 30, 2016

Choose a reason for hiding this comment

maxrimue commented May 27, 2016

nexdrew commented May 27, 2016

nexdrew commented May 27, 2016

maxrimue commented May 27, 2016

coveralls commented May 27, 2016 • edited

bcoe May 30, 2016

Choose a reason for hiding this comment

bcoe commented Jul 9, 2016

bcoe commented Jul 9, 2016

AGresvig commented Jul 11, 2016

nexdrew commented May 3, 2016 •

edited

coveralls commented May 3, 2016 •

edited

coveralls commented May 23, 2016 •

edited

coveralls commented May 24, 2016 •

edited

coveralls commented May 27, 2016 •

edited