From 39cb757330b4ff66edd44032be8a7dc8986fc1ad Mon Sep 17 00:00:00 2001 From: John Gee Date: Sat, 13 Mar 2021 17:13:09 +1300 Subject: [PATCH 1/4] Parse command-argument details in constructor --- index.js | 131 +++++++++++------------------ tests/args.badArg.test.js | 4 +- tests/help.visibleCommands.test.js | 4 +- 3 files changed, 55 insertions(+), 84 deletions(-) diff --git a/index.js b/index.js index 2c560bca3..6772bf885 100644 --- a/index.js +++ b/index.js @@ -28,11 +28,11 @@ class Help { const visibleCommands = cmd.commands.filter(cmd => !cmd._hidden); if (cmd._hasImplicitHelpCommand()) { // Create a command matching the implicit help command. - const args = cmd._helpCommandnameAndArgs.split(/ +/); - const helpCommand = cmd.createCommand(args.shift()) + const pieces = cmd._helpCommandnameAndArgs.split(/ +/); + const helpCommand = cmd.createCommand(pieces.shift()) .helpOption(false); helpCommand.description(cmd._helpCommandDescription); - helpCommand._parseExpectedArgs(args); + if (pieces.length > 0) helpCommand.arguments(pieces.join(' ')); visibleCommands.push(helpCommand); } if (this.sortSubcommands) { @@ -346,22 +346,35 @@ class Help { class Argument { /** - * Initialize a new argument with description. + * Initialize a new argument with the given detail and description. * - * @param {string} arg - * @param {object} [description] + * @param {string} detail + * @param {string} [description] */ - constructor(arg, description) { - const argDetails = parseArg(arg); - if (argDetails === undefined) { - throw new Error(`Bad argument format: ${arg}`); + constructor(detail, description) { + this.required = false; + this.variadic = false; + this.name = ''; + this.description = description || ''; + + switch (detail[0]) { + case '<': // e.g. + this.required = true; + this.name = detail.slice(1, -1); + break; + case '[': // e.g. [optional] + this.name = detail.slice(1, -1); + break; + } + + if (this.name.length > 3 && this.name.slice(-3) === '...') { + this.variadic = true; + this.name = this.name.slice(0, -3); } - if (argDetails) { - this.name = argDetails.name; - this.required = argDetails.required; - this.variadic = argDetails.variadic; - this.description = description || ''; + + if (this.name.length === 0) { + throw new Error(`Unrecognised argument format (expecting '' or '[optional]'): ${detail}`); } } } @@ -662,7 +675,7 @@ class Command extends EventEmitter { cmd._executableFile = opts.executableFile || null; // Custom name for executable file, set missing to null to match constructor this.commands.push(cmd); - cmd._parseExpectedArgs(args); + if (args.length > 0) cmd.arguments(args.join(' ')); cmd.parent = this; if (desc) return this; @@ -773,8 +786,11 @@ class Command extends EventEmitter { * Define argument syntax for the command. */ - arguments(desc) { - return this._parseExpectedArgs(desc.split(/ +/)); + arguments(details) { + details.split(/ +/).forEach((detail) => { + this.argument(detail); + }); + return this; }; /** @@ -782,22 +798,31 @@ class Command extends EventEmitter { * @param {Argument} argument */ addArgument(argument) { + const previousArgument = this._args.slice(-1)[0]; + if (previousArgument && previousArgument.variadic) { + throw new Error(`only the last argument can be variadic '${previousArgument.name}'`); + } + this._args.push(argument); - if (!this._argsDescription) { - this._argsDescription = {}; + + // Legacy description handling + if (argument.description) { + if (!this._argsDescription) { + this._argsDescription = {}; + } + this._argsDescription[argument.name] = argument.description; } - this._argsDescription[argument.name] = argument.description; - this._validateArgs(); + return this; } /** * Define argument syntax for the command - * @param {string} arg - * @param {object} [description] + * @param {string} detail + * @param {string} [description] */ - argument(arg, description) { - const argument = new Argument(arg, description); + argument(detail, description) { + const argument = new Argument(detail, description); this.addArgument(argument); return this; } @@ -838,34 +863,6 @@ class Command extends EventEmitter { return this._addImplicitHelpCommand; }; - /** - * Parse expected `args`. - * - * For example `["[type]"]` becomes `[{ required: false, name: 'type' }]`. - * - * @param {Array} args - * @return {Command} `this` command for chaining - * @api private - */ - - _parseExpectedArgs(args) { - if (!args.length) return; - args.forEach((arg) => { - const argDetails = parseArg(arg); - argDetails && this._args.push(argDetails); - }); - this._validateArgs(); - return this; - }; - - _validateArgs() { - this._args.forEach((arg, i) => { - if (arg.variadic && i < this._args.length - 1) { - throw new Error(`only the last argument can be variadic '${arg.name}'`); - } - }); - } - /** * Register callback to use as replacement for calling process.exit. * @@ -2231,29 +2228,3 @@ function incrementNodeInspectorPort(args) { return arg; }); } - -function parseArg(arg) { - const argDetails = { - required: false, - name: '', - variadic: false - }; - - switch (arg[0]) { - case '<': - argDetails.required = true; - argDetails.name = arg.slice(1, -1); - break; - case '[': - argDetails.name = arg.slice(1, -1); - break; - } - - if (argDetails.name.length > 3 && argDetails.name.slice(-3) === '...') { - argDetails.variadic = true; - argDetails.name = argDetails.name.slice(0, -3); - } - if (argDetails.name) { - return argDetails; - } -} diff --git a/tests/args.badArg.test.js b/tests/args.badArg.test.js index 0d0b61b1f..3ec3e4a31 100644 --- a/tests/args.badArg.test.js +++ b/tests/args.badArg.test.js @@ -2,6 +2,6 @@ const commander = require('../'); test('should throw on bad argument', () => { const program = new commander.Command(); - expect(() => program.addArgument(new commander.Argument('bad name'))).toThrowError('Bad argument format: bad name'); - expect(() => program.argument(new commander.Argument('bad name'))).toThrowError('Bad argument format: bad name'); + expect(() => program.addArgument(new commander.Argument('bad-format'))).toThrow(); + expect(() => program.argument('bad-format')).toThrow(); }); diff --git a/tests/help.visibleCommands.test.js b/tests/help.visibleCommands.test.js index bc8aed7e0..3ea8cefcc 100644 --- a/tests/help.visibleCommands.test.js +++ b/tests/help.visibleCommands.test.js @@ -23,9 +23,9 @@ describe('visibleCommands', () => { const program = new commander.Command(); program .command('visible', 'desc') - .command('invisible executable', 'desc', { hidden: true }); + .command('invisible-executable', 'desc', { hidden: true }); program - .command('invisible action', { hidden: true }); + .command('invisible-action', { hidden: true }); const helper = new commander.Help(); const visibleCommandNames = helper.visibleCommands(program).map(cmd => cmd.name()); expect(visibleCommandNames).toEqual(['visible', 'help']); From 8c4d8d0a415f4327387e29c25f263048bd71b1db Mon Sep 17 00:00:00 2001 From: John Gee Date: Sat, 13 Mar 2021 18:38:53 +1300 Subject: [PATCH 2/4] Handle argument descriptions separately for legacy and new support --- index.js | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index 6772bf885..3f3b7ba19 100644 --- a/index.js +++ b/index.js @@ -86,11 +86,15 @@ class Help { */ visibleArguments(cmd) { - if (cmd._argsDescription && cmd._args.length) { - return cmd._args.map((argument) => { - return { term: argument.name, description: cmd._argsDescription[argument.name] || '' }; - }, 0); - } + // If there are some argument description then return all the arguments. + if (cmd._argsDescription || cmd._args.find(argument => argument.description)) { + const legacyDescriptions = cmd._argsDescription || {}; + return cmd._args.map(argument => { + const term = argument.name; + const description = argument.description || legacyDescriptions[argument.name] || ''; + return { term, description }; + }); + }; return []; } @@ -586,7 +590,7 @@ class Command extends EventEmitter { this._aliases = []; this._combineFlagAndOptionalValue = true; this._description = ''; - this._argsDescription = undefined; + this._argsDescription = undefined; // legacy this._enablePositionalOptions = false; this._passThroughOptions = false; @@ -802,17 +806,7 @@ class Command extends EventEmitter { if (previousArgument && previousArgument.variadic) { throw new Error(`only the last argument can be variadic '${previousArgument.name}'`); } - this._args.push(argument); - - // Legacy description handling - if (argument.description) { - if (!this._argsDescription) { - this._argsDescription = {}; - } - this._argsDescription[argument.name] = argument.description; - } - return this; } From c2e0a5a9284aaccc7edb84b817cd2fbbbf519b92 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sat, 13 Mar 2021 21:31:32 +1300 Subject: [PATCH 3/4] Add text to distinguish test names with .each --- tests/command.action.test.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/command.action.test.js b/tests/command.action.test.js index bf0d34a6b..765dddf0c 100644 --- a/tests/command.action.test.js +++ b/tests/command.action.test.js @@ -23,14 +23,14 @@ test('when .action called then program.args only contains args', () => { expect(program.args).toEqual(['info', 'my-file']); }); -test.each(getTestCases(''))('when .action on program with required argument and argument supplied then action called', (program) => { +test.each(getTestCases(''))('when .action on program with required argument via %s and argument supplied then action called', (methodName, program) => { const actionMock = jest.fn(); program.action(actionMock); program.parse(['node', 'test', 'my-file']); expect(actionMock).toHaveBeenCalledWith('my-file', program.opts(), program); }); -test.each(getTestCases(''))('when .action on program with required argument and argument not supplied then action not called', (program) => { +test.each(getTestCases(''))('when .action on program with required argument via %s and argument not supplied then action not called', (methodName, program) => { const actionMock = jest.fn(); program .exitOverride() @@ -52,21 +52,21 @@ test('when .action on program and no arguments then action called', () => { expect(actionMock).toHaveBeenCalledWith(program.opts(), program); }); -test.each(getTestCases('[file]'))('when .action on program with optional argument supplied then action called', (program) => { +test.each(getTestCases('[file]'))('when .action on program with optional argument via %s supplied then action called', (methodName, program) => { const actionMock = jest.fn(); program.action(actionMock); program.parse(['node', 'test', 'my-file']); expect(actionMock).toHaveBeenCalledWith('my-file', program.opts(), program); }); -test.each(getTestCases('[file]'))('when .action on program without optional argument supplied then action called', (program) => { +test.each(getTestCases('[file]'))('when .action on program without optional argument supplied then action called', (methodName, program) => { const actionMock = jest.fn(); program.action(actionMock); program.parse(['node', 'test']); expect(actionMock).toHaveBeenCalledWith(undefined, program.opts(), program); }); -test.each(getTestCases('[file]'))('when .action on program with optional argument and subcommand and program argument then program action called', (program) => { +test.each(getTestCases('[file]'))('when .action on program with optional argument via %s and subcommand and program argument then program action called', (methodName, program) => { const actionMock = jest.fn(); program.action(actionMock); program @@ -78,7 +78,7 @@ test.each(getTestCases('[file]'))('when .action on program with optional argumen }); // Changes made in #1062 to allow this case -test.each(getTestCases('[file]'))('when .action on program with optional argument and subcommand and no program argument then program action called', (program) => { +test.each(getTestCases('[file]'))('when .action on program with optional argument via %s and subcommand and no program argument then program action called', (methodName, program) => { const actionMock = jest.fn(); program.action(actionMock); program.command('subcommand'); @@ -108,5 +108,5 @@ function getTestCases(arg) { const withArguments = new commander.Command().arguments(arg); const withArgument = new commander.Command().argument(arg); const withAddArgument = new commander.Command().addArgument(new commander.Argument(arg)); - return [withArguments, withArgument, withAddArgument]; + return [['.arguments', withArguments], ['.argument', withArgument], ['.addArgument', withAddArgument]]; } From 32f001bfdc8c592d637754144bb97d52849d1500 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 14 Mar 2021 19:02:03 +1300 Subject: [PATCH 4/4] Match nameAndArgs into two parts, rather than split on spaces. --- index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index 3f3b7ba19..848240746 100644 --- a/index.js +++ b/index.js @@ -28,11 +28,11 @@ class Help { const visibleCommands = cmd.commands.filter(cmd => !cmd._hidden); if (cmd._hasImplicitHelpCommand()) { // Create a command matching the implicit help command. - const pieces = cmd._helpCommandnameAndArgs.split(/ +/); - const helpCommand = cmd.createCommand(pieces.shift()) + const [, helpName, helpArgs] = cmd._helpCommandnameAndArgs.match(/([^ ]+) *(.*)/); + const helpCommand = cmd.createCommand(helpName) .helpOption(false); helpCommand.description(cmd._helpCommandDescription); - if (pieces.length > 0) helpCommand.arguments(pieces.join(' ')); + if (helpArgs) helpCommand.arguments(helpArgs); visibleCommands.push(helpCommand); } if (this.sortSubcommands) { @@ -650,8 +650,8 @@ class Command extends EventEmitter { desc = null; } opts = opts || {}; - const args = nameAndArgs.split(/ +/); - const cmd = this.createCommand(args.shift()); + const [, name, args] = nameAndArgs.match(/([^ ]+) *(.*)/); + const cmd = this.createCommand(name); if (desc) { cmd.description(desc); @@ -678,8 +678,8 @@ class Command extends EventEmitter { cmd._enablePositionalOptions = this._enablePositionalOptions; cmd._executableFile = opts.executableFile || null; // Custom name for executable file, set missing to null to match constructor + if (args) cmd.arguments(args); this.commands.push(cmd); - if (args.length > 0) cmd.arguments(args.join(' ')); cmd.parent = this; if (desc) return this;