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.

Sign up for GitHub

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

Jump to bottom

Check command-arguments even without action handler #1502

Merged
merged 2 commits into from Apr 11, 2021
Merged

Check command-arguments even without action handler #1502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5737c2
Check command-arguments even without action handler
shadowspawn Apr 10, 2021
ad83ea5
Separate checking number of arguments and collecting variadic
shadowspawn Apr 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 25 additions & 13 deletions index.js
View file
Open in desktop
Expand Up @@ -1559,28 +1559,38 @@ class Command extends EventEmitter {
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();
const checkNumberOfArguments = () => {
// too few
this._args.forEach((arg, i) => {
if (arg.required && args[i] == null) {
if (arg.required && this.args[i] == null) {
this.missingArgument(arg.name());
} else if (arg.variadic) {
args[i] = args.splice(i);
args.length = Math.min(i + 1, args.length);
}
});
if (args.length > this._args.length) {
this._excessArguments(args);
// too many
if (this._args.length > 0 && this._args[this._args.length - 1].variadic) {
return;
}
if (this.args.length > this._args.length) {
this._excessArguments(this.args);
}
};

this._actionHandler(args);
const commandEvent = `command:${this.name()}`;
if (this._actionHandler) {
checkForUnknownOptions();
checkNumberOfArguments();
// Collect trailing args into variadic.
let actionArgs = this.args;
const declaredArgCount = this._args.length;
if (declaredArgCount > 0 && this._args[declaredArgCount - 1].variadic) {
actionArgs = this.args.slice(0, declaredArgCount - 1);
actionArgs[declaredArgCount - 1] = this.args.slice(declaredArgCount - 1);
}
this._actionHandler(actionArgs);
if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy
} else if (this.parent && this.parent.listenerCount(commandEvent)) {
checkForUnknownOptions();
checkNumberOfArguments();
this.parent.emit(commandEvent, operands, unknown); // legacy
} else if (operands.length) {
if (this._findCommand('*')) { // legacy default command
Expand All @@ -1592,12 +1602,14 @@ class Command extends EventEmitter {
this.unknownCommand();
} else {
checkForUnknownOptions();
checkNumberOfArguments();
}
} 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();
checkNumberOfArguments();
// fall through for caller to handle after calling .parse()
}
}
Expand Down
80 changes: 32 additions & 48 deletions tests/command.allowExcessArguments.test.js
View file
Open in desktop
Expand Up @@ -2,27 +2,21 @@ const commander = require('../');

// Not testing output, just testing whether an error is detected.

describe('allowUnknownOption', () => {
// Optional. Use internal knowledge to suppress output to keep test output clean.
let writeErrorSpy;

beforeAll(() => {
writeErrorSpy = jest.spyOn(process.stderr, 'write').mockImplementation(() => { });
});

afterEach(() => {
writeErrorSpy.mockClear();
});

afterAll(() => {
writeErrorSpy.mockRestore();
});
describe.each([true, false])('allowExcessArguments when action handler: %s', (hasActionHandler) => {
function configureCommand(cmd) {
cmd
.exitOverride()
.configureOutput({
writeErr: () => {}
});
if (hasActionHandler) {
cmd.action(() => {});
}
}

test('when specify excess program argument then no error by default', () => {
const program = new commander.Command();
program
.exitOverride()
.action(() => {});
configureCommand(program);

expect(() => {
program.parse(['excess'], { from: 'user' });
Expand All @@ -31,10 +25,9 @@ describe('allowUnknownOption', () => {

test('when specify excess program argument and allowExcessArguments(false) then error', () => {
const program = new commander.Command();
configureCommand(program);
program
.exitOverride()
.allowExcessArguments(false)
.action(() => {});
.allowExcessArguments(false);

expect(() => {
program.parse(['excess'], { from: 'user' });
Expand All @@ -43,10 +36,9 @@ describe('allowUnknownOption', () => {

test('when specify excess program argument and allowExcessArguments() then no error', () => {
const program = new commander.Command();
configureCommand(program);
program
.exitOverride()
.allowExcessArguments()
.action(() => {});
.allowExcessArguments();

expect(() => {
program.parse(['excess'], { from: 'user' });
Expand All @@ -55,10 +47,9 @@ describe('allowUnknownOption', () => {

test('when specify excess program argument and allowExcessArguments(true) then no error', () => {
const program = new commander.Command();
configureCommand(program);
program
.exitOverride()
.allowExcessArguments(true)
.action(() => {});
.allowExcessArguments(true);

expect(() => {
program.parse(['excess'], { from: 'user' });
Expand All @@ -67,10 +58,9 @@ describe('allowUnknownOption', () => {

test('when specify excess command argument then no error (by default)', () => {
const program = new commander.Command();
program
.exitOverride()
.command('sub')
.action(() => { });
const sub = program
.command('sub');
configureCommand(sub);

expect(() => {
program.parse(['sub', 'excess'], { from: 'user' });
Expand All @@ -79,12 +69,10 @@ describe('allowUnknownOption', () => {

test('when specify excess command argument and allowExcessArguments(false) then error', () => {
const program = new commander.Command();
program
.exitOverride()
const sub = program
.command('sub')
.allowUnknownOption()
.allowExcessArguments(false)
.action(() => { });
.allowExcessArguments(false);
configureCommand(sub);

expect(() => {
program.parse(['sub', 'excess'], { from: 'user' });
Expand All @@ -93,11 +81,10 @@ describe('allowUnknownOption', () => {

test('when specify expected arg and allowExcessArguments(false) then no error', () => {
const program = new commander.Command();
configureCommand(program);
program
.argument('<file>')
.exitOverride()
.allowExcessArguments(false)
.action(() => {});
.allowExcessArguments(false);

expect(() => {
program.parse(['file'], { from: 'user' });
Expand All @@ -106,11 +93,10 @@ describe('allowUnknownOption', () => {

test('when specify excess after <arg> and allowExcessArguments(false) then error', () => {
const program = new commander.Command();
configureCommand(program);
program
.argument('<file>')
.exitOverride()
.allowExcessArguments(false)
.action(() => {});
.allowExcessArguments(false);

expect(() => {
program.parse(['file', 'excess'], { from: 'user' });
Expand All @@ -119,11 +105,10 @@ describe('allowUnknownOption', () => {

test('when specify excess after [arg] and allowExcessArguments(false) then error', () => {
const program = new commander.Command();
configureCommand(program);
program
.argument('[file]')
.exitOverride()
.allowExcessArguments(false)
.action(() => {});
.allowExcessArguments(false);

expect(() => {
program.parse(['file', 'excess'], { from: 'user' });
Expand All @@ -132,11 +117,10 @@ describe('allowUnknownOption', () => {

test('when specify args for [args...] and allowExcessArguments(false) then no error', () => {
const program = new commander.Command();
configureCommand(program);
program
.argument('[files...]')
.exitOverride()
.allowExcessArguments(false)
.action(() => {});
.allowExcessArguments(false);

expect(() => {
program.parse(['file1', 'file2', 'file3'], { from: 'user' });
Expand Down
17 changes: 17 additions & 0 deletions tests/command.exitOverride.test.js
View file
Open in desktop
Expand Up @@ -120,6 +120,23 @@ describe('.exitOverride and error details', () => {
expectCommanderError(caughtErr, 1, 'commander.missingArgument', "error: missing required argument 'arg-name'");
});

test('when specify program without required argument and no action handler then throw CommanderError', () => {
const program = new commander.Command();
program
.exitOverride()
.argument('<arg-name>');

let caughtErr;
try {
program.parse(['node', 'test']);
} catch (err) {
caughtErr = err;
}

expect(stderrSpy).toHaveBeenCalled();
expectCommanderError(caughtErr, 1, 'commander.missingArgument', "error: missing required argument 'arg-name'");
});

test('when specify excess argument then throw CommanderError', () => {
const program = new commander.Command();
program
Expand Down