Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip unknown options check if there is a better error to display #1464

Merged
18 changes: 14 additions & 4 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1475,12 +1475,17 @@ class Command extends EventEmitter {

outputHelpIfRequested(this, parsed.unknown);
this._checkForMissingMandatoryOptions();
if (parsed.unknown.length > 0) {
this.unknownOption(parsed.unknown[0]);
}

// 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]);
}
};

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) => {
Expand All @@ -1498,19 +1503,24 @@ 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) {
if (this._findCommand('*')) { // legacy
if (this._findCommand('*')) { // legacy default command
this._dispatchSubcommand('*', operands, unknown);
} 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();
} else {
checkForUnknownOptions();
}
} else if (this.commands.length) {
// 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()
}
}
Expand Down
53 changes: 53 additions & 0 deletions tests/command.asterisk.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,45 @@ describe(".command('*')", () => {
program.parse(['node', 'test', 'unrecognised-command']);
expect(mockAction).toHaveBeenCalled();
});

test('when unrecognised argument and known option then asterisk action called', () => {
// 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
.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 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);
let caughtErr;
try {
program.parse(['node', 'test', 'some-argument', '--unknown']);
} catch (err) {
caughtErr = err;
}
expect(caughtErr.code).toEqual('commander.unknownOption');
});
});

// Test .on explicitly rather than assuming covered by .command
Expand Down Expand Up @@ -108,4 +147,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();
});
});
18 changes: 17 additions & 1 deletion tests/command.unknownCommand.test.js
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -63,6 +63,22 @@ 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 command had been correctly spelled, say).
const program = new commander.Command();
program
.exitOverride()
.command('sub');
let caughtErr;
try {
program.parse('node test.js sbu --silly'.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
Expand Down
13 changes: 13 additions & 0 deletions tests/command.unknownOption.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});