From d002aea8b7df6acd9a3e5016d9e0ad273b0a3ad0 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 13:38:38 +1300 Subject: [PATCH 01/13] Check for uknown options in multiple places, to allow skipping check --- index.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 641946960..e44a5e6c5 100644 --- a/index.js +++ b/index.js @@ -1475,12 +1475,16 @@ class Command extends EventEmitter { outputHelpIfRequested(this, parsed.unknown); this._checkForMissingMandatoryOptions(); - if (parsed.unknown.length > 0) { - this.unknownOption(parsed.unknown[0]); - } + + const checkForUnknownOptions = () => { + if (parsed.unknown.length > 0) { + this.unknownOption(parsed.unknown[0]); + } + }; const commandEvent = `command:${this.name()}`; if (this._actionHandler) { + checkForUnknownOptions(); // Check expected arguments and collect variadic together. const args = this.args.slice(); this._args.forEach((arg, i) => { @@ -1498,8 +1502,10 @@ class Command extends EventEmitter { this._actionHandler(args); if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy } else if (this.parent && this.parent.listenerCount(commandEvent)) { + checkForUnknownOptions(); this.parent.emit(commandEvent, operands, unknown); // legacy } else if (operands.length) { + checkForUnknownOptions(); if (this._findCommand('*')) { // legacy this._dispatchSubcommand('*', operands, unknown); } else if (this.listenerCount('command:*')) { @@ -1508,9 +1514,11 @@ class Command extends EventEmitter { this.unknownCommand(); } } else if (this.commands.length) { + checkForUnknownOptions(); // This command has subcommands and nothing hooked up at this level, so display help. this.help({ error: true }); } else { + checkForUnknownOptions(); // fall through for caller to handle after calling .parse() } } From 4aeb0aea6021462695d234581b42c89a852fbb1d Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 13:45:17 +1300 Subject: [PATCH 02/13] Missing command shows help, does not check for options --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index e44a5e6c5..67f8912b4 100644 --- a/index.js +++ b/index.js @@ -1514,7 +1514,6 @@ class Command extends EventEmitter { this.unknownCommand(); } } else if (this.commands.length) { - checkForUnknownOptions(); // This command has subcommands and nothing hooked up at this level, so display help. this.help({ error: true }); } else { From 7e3756894e00144dabc0ccb65b07cd8e0c3b667c Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 13:52:08 +1300 Subject: [PATCH 03/13] Check unknown commands before unknown options --- index.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 67f8912b4..27457b6ce 100644 --- a/index.js +++ b/index.js @@ -1505,13 +1505,15 @@ class Command extends EventEmitter { checkForUnknownOptions(); this.parent.emit(commandEvent, operands, unknown); // legacy } else if (operands.length) { - checkForUnknownOptions(); - if (this._findCommand('*')) { // legacy + if (this._findCommand('*')) { // legacy default command + checkForUnknownOptions(); this._dispatchSubcommand('*', operands, unknown); - } else if (this.listenerCount('command:*')) { + } else if (this.listenerCount('command:*')) { // suggestions this.emit('command:*', operands, unknown); } else if (this.commands.length) { this.unknownCommand(); + } else { + checkForUnknownOptions(); } } else if (this.commands.length) { // This command has subcommands and nothing hooked up at this level, so display help. From 8b7f32e516beb61c55e801e35067d642862f53c4 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 14:06:25 +1300 Subject: [PATCH 04/13] Add comments --- index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 27457b6ce..8daaf9cb6 100644 --- a/index.js +++ b/index.js @@ -1476,6 +1476,7 @@ class Command extends EventEmitter { outputHelpIfRequested(this, parsed.unknown); this._checkForMissingMandatoryOptions(); + // We do not always call this check to avoid masking a "better" error, like unknown command. const checkForUnknownOptions = () => { if (parsed.unknown.length > 0) { this.unknownOption(parsed.unknown[0]); @@ -1508,7 +1509,8 @@ class Command extends EventEmitter { if (this._findCommand('*')) { // legacy default command checkForUnknownOptions(); this._dispatchSubcommand('*', operands, unknown); - } else if (this.listenerCount('command:*')) { // suggestions + } else if (this.listenerCount('command:*')) { + // skip option check, emit event for possible misspelling suggestion this.emit('command:*', operands, unknown); } else if (this.commands.length) { this.unknownCommand(); From 0ccd7483f8c4ecfc89e234025cd963013fd21088 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 16:43:01 +1300 Subject: [PATCH 05/13] Restore legacy command:* handling --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index 8daaf9cb6..ec9887905 100644 --- a/index.js +++ b/index.js @@ -1507,7 +1507,6 @@ class Command extends EventEmitter { this.parent.emit(commandEvent, operands, unknown); // legacy } else if (operands.length) { if (this._findCommand('*')) { // legacy default command - checkForUnknownOptions(); this._dispatchSubcommand('*', operands, unknown); } else if (this.listenerCount('command:*')) { // skip option check, emit event for possible misspelling suggestion From b63a259977ff75f8e2c4a7ee32bb2ee0daaa6053 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 17:13:12 +1300 Subject: [PATCH 06/13] Add more tests for legacy command('*') --- tests/command.asterisk.test.js | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/command.asterisk.test.js b/tests/command.asterisk.test.js index cf0e7f004..13541f3b5 100644 --- a/tests/command.asterisk.test.js +++ b/tests/command.asterisk.test.js @@ -64,6 +64,37 @@ describe(".command('*')", () => { program.parse(['node', 'test', 'unrecognised-command']); expect(mockAction).toHaveBeenCalled(); }); + + test('when unrecognised argument and known option then asterisk action called', () => { + const mockAction = jest.fn(); + const program = new commander.Command(); + program + .command('install'); + const star = program + .command('*') + .arguments('[args...]') + .option('-d, --debug') + .action(mockAction); + program.parse(['node', 'test', 'unrecognised-command', '--debug']); + expect(mockAction).toHaveBeenCalled(); + expect(star.opts().debug).toEqual(true); + }); + + test('when unrecognised argument and unknown option then error', () => { + // This is a change in behaviour from v2, but is consistent with modern better detection of invalid options + const mockAction = jest.fn(); + const program = new commander.Command(); + program + .exitOverride() + .command('install'); + program + .command('*') + .arguments('[args...]') + .action(mockAction); + expect(() => { + program.parse(['node', 'test', 'unrecognised-command', '--unknown']); + }).toThrow(); + }); }); // Test .on explicitly rather than assuming covered by .command From b588a1a2103919a5ff4392bc1da65e06f3d81299 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 17:16:47 +1300 Subject: [PATCH 07/13] Add comment explaining history for test --- tests/command.asterisk.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/command.asterisk.test.js b/tests/command.asterisk.test.js index 13541f3b5..fe6aebafb 100644 --- a/tests/command.asterisk.test.js +++ b/tests/command.asterisk.test.js @@ -66,6 +66,7 @@ describe(".command('*')", () => { }); test('when unrecognised argument and known option then asterisk action called', () => { + // This tests for a regression between v4 and v5. Unknown option should not be detected by program. const mockAction = jest.fn(); const program = new commander.Command(); program From 05671f1ff3c2e9a22bd0d7a47074157184fcd727 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 17:20:58 +1300 Subject: [PATCH 08/13] Improve comment --- tests/command.asterisk.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/command.asterisk.test.js b/tests/command.asterisk.test.js index fe6aebafb..ec3bbf4e1 100644 --- a/tests/command.asterisk.test.js +++ b/tests/command.asterisk.test.js @@ -66,7 +66,7 @@ describe(".command('*')", () => { }); test('when unrecognised argument and known option then asterisk action called', () => { - // This tests for a regression between v4 and v5. Unknown option should not be detected by program. + // This tests for a regression between v4 and v5. Known default option should not be rejected by program. const mockAction = jest.fn(); const program = new commander.Command(); program From 4a9ca4954d7f6aff3a30013964ffeaa91aa516cb Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 22:13:35 +1300 Subject: [PATCH 09/13] Test for command suggestion despite unknown option --- tests/command.asterisk.test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/command.asterisk.test.js b/tests/command.asterisk.test.js index ec3bbf4e1..1bdee57fd 100644 --- a/tests/command.asterisk.test.js +++ b/tests/command.asterisk.test.js @@ -140,4 +140,18 @@ describe(".on('command:*')", () => { program.parse(['node', 'test', 'unrecognised-command']); expect(mockAction).toHaveBeenCalled(); }); + + test('when unrecognised command/argument and unknown option then listener called', () => { + // Give listener a chance to make a suggestion for misspelled command. The option + // could only be unknown because the command is not correct. + // Regression identified in https://github.com/tj/commander.js/issues/1460#issuecomment-772313494 + const mockAction = jest.fn(); + const program = new commander.Command(); + program + .command('install'); + program + .on('command:*', mockAction); + program.parse(['node', 'test', 'intsall', '--unknown']); + expect(mockAction).toHaveBeenCalled(); + }); }); From 118d7440c396e38021e6dce34c05273a86894a63 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 22:32:22 +1300 Subject: [PATCH 10/13] Improve comments and suppress test output --- tests/command.asterisk.test.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/command.asterisk.test.js b/tests/command.asterisk.test.js index 1bdee57fd..d975c868b 100644 --- a/tests/command.asterisk.test.js +++ b/tests/command.asterisk.test.js @@ -81,20 +81,27 @@ describe(".command('*')", () => { expect(star.opts().debug).toEqual(true); }); - test('when unrecognised argument and unknown option then error', () => { - // This is a change in behaviour from v2, but is consistent with modern better detection of invalid options + test('when non-command argument and unknown option then error for unknown option', () => { + // This is a change in behaviour from v2 which did not error, but is consistent with modern better detection of invalid options const mockAction = jest.fn(); const program = new commander.Command(); program .exitOverride() + .configureOutput({ + writeErr: () => {} + }) .command('install'); program .command('*') .arguments('[args...]') .action(mockAction); - expect(() => { - program.parse(['node', 'test', 'unrecognised-command', '--unknown']); - }).toThrow(); + let caughtErr; + try { + program.parse(['node', 'test', 'some-argument', '--unknown']); + } catch (err) { + caughtErr = err; + } + expect(caughtErr.code).toEqual('commander.unknownOption'); }); }); @@ -144,7 +151,7 @@ describe(".on('command:*')", () => { test('when unrecognised command/argument and unknown option then listener called', () => { // Give listener a chance to make a suggestion for misspelled command. The option // could only be unknown because the command is not correct. - // Regression identified in https://github.com/tj/commander.js/issues/1460#issuecomment-772313494 + // Regression identified in https://github.com/tj/commander.js/issues/1460#issuecomment-772313494 const mockAction = jest.fn(); const program = new commander.Command(); program From 8ae775ce660fed1a7a68abc23445bf670888c824 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 22:32:51 +1300 Subject: [PATCH 11/13] Add test for competing unknown command and option --- tests/command.unknownCommand.test.js | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/command.unknownCommand.test.js b/tests/command.unknownCommand.test.js index 844c8455a..82de161db 100644 --- a/tests/command.unknownCommand.test.js +++ b/tests/command.unknownCommand.test.js @@ -1,6 +1,6 @@ const commander = require('../'); -describe('unknownOption', () => { +describe('unknownCommand', () => { // Optional. Use internal knowledge to suppress output to keep test output clean. let writeErrorSpy; @@ -63,6 +63,23 @@ describe('unknownOption', () => { expect(caughtErr.code).toBe('commander.unknownCommand'); }); + test('when unknown command and unknown option then error is for unknown command', () => { + // The unknown command is more useful since the option is for an unknown command (and might be + // ok if the commadn had been correctly spelled, say). + const program = new commander.Command(); + program + .exitOverride() + .command('sub') + .option('-d, --debug'); + let caughtErr; + try { + program.parse('node test.js sbu --debug'.split(' ')); + } catch (err) { + caughtErr = err; + } + expect(caughtErr.code).toBe('commander.unknownCommand'); + }); + test('when unknown subcommand then help suggestion includes command path', () => { const program = new commander.Command(); program From 2c152710e5abfff3ccb75d6c526b1e2a2a275102 Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 22:48:38 +1300 Subject: [PATCH 12/13] Test that unknown option is an error in simple program --- tests/command.unknownOption.test.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/command.unknownOption.test.js b/tests/command.unknownOption.test.js index 18ed188a5..4bceeeb8f 100644 --- a/tests/command.unknownOption.test.js +++ b/tests/command.unknownOption.test.js @@ -82,4 +82,17 @@ describe('unknownOption', () => { } expect(caughtErr.code).toBe('commander.unknownOption'); }); + + test('when specify unknown option with simple program then error', () => { + const program = new commander.Command(); + program + .exitOverride(); + let caughtErr; + try { + program.parse(['node', 'test', '--NONSENSE']); + } catch (err) { + caughtErr = err; + } + expect(caughtErr.code).toBe('commander.unknownOption'); + }); }); From 877a6038bf87ded888d118eee1c682ccdcc807ed Mon Sep 17 00:00:00 2001 From: John Gee Date: Fri, 5 Feb 2021 23:11:18 +1300 Subject: [PATCH 13/13] Fix test and comment --- tests/command.unknownCommand.test.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/command.unknownCommand.test.js b/tests/command.unknownCommand.test.js index 82de161db..7e8bfa0d9 100644 --- a/tests/command.unknownCommand.test.js +++ b/tests/command.unknownCommand.test.js @@ -65,15 +65,14 @@ describe('unknownCommand', () => { test('when unknown command and unknown option then error is for unknown command', () => { // The unknown command is more useful since the option is for an unknown command (and might be - // ok if the commadn had been correctly spelled, say). + // ok if the command had been correctly spelled, say). const program = new commander.Command(); program .exitOverride() - .command('sub') - .option('-d, --debug'); + .command('sub'); let caughtErr; try { - program.parse('node test.js sbu --debug'.split(' ')); + program.parse('node test.js sbu --silly'.split(' ')); } catch (err) { caughtErr = err; }