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

Add .allowExcessArguments() and error message #1407

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,7 @@ class Command extends EventEmitter {
this.options = [];
this.parent = null;
this._allowUnknownOption = false;
this._allowExcessArguments = true;
this._args = [];
this.rawArgs = null;
this._scriptPath = null;
Expand Down Expand Up @@ -1170,6 +1171,17 @@ Read more on https://git.io/JJc0W`);
return this;
};

/**
* Allow excess arguments on the command line.
*
* @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown
* for excess arguments.
*/
allowExcessArguments(allowExcess) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the default parameters available?

allowExcessArguments(allowExcess = true) 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'll look into that. We have a few methods with this sort of pattern that could use it. Likely can because we have moved forward our minimum version of node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults from node 6, which is our minimum supported version: https://node.green/#ES2015-syntax-default-function-parameters

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added default parameter instead of manual test to multiple routines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing it.

this._allowExcessArguments = (allowExcess === undefined) || !!allowExcess;
return this;
};

/**
* Whether to store option values as properties on command object,
* or store separately (specify false). In both cases the option values can be accessed using .opts().
Expand Down Expand Up @@ -1492,14 +1504,19 @@ Read more on https://git.io/JJc0W`);

const commandEvent = `command:${this.name()}`;
if (this._actionHandler) {
// Check expected arguments and collect variadic together.
const args = this.args.slice();
this._args.forEach((arg, i) => {
if (arg.required && 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);
}

this._actionHandler(args);
if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy
Expand Down Expand Up @@ -1753,6 +1770,23 @@ Read more on https://git.io/JJc0W`);
this._displayError(1, 'commander.unknownOption', message);
};

/**
* Excess arguments, more than expected.
*
* @param {string[]} receivedArgs
* @api private
*/

_excessArguments(receivedArgs) {
if (this._allowExcessArguments) return;

const expected = this._args.length;
const s = (expected === 1) ? '' : 's';
const forSubcommand = this.parent ? ` for '${this.name()}'` : '';
const message = `error: too many arguments${forSubcommand}. Expected ${expected} argument${s} but got ${receivedArgs.length}.`;
this._displayError(1, 'commander.excessArguments', message);
};

/**
* Unknown command.
*
Expand Down
145 changes: 145 additions & 0 deletions tests/command.allowExcessArguments.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
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();
});

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

expect(() => {
program.parse(['excess'], { from: 'user' });
}).not.toThrow();
});

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

expect(() => {
program.parse(['excess'], { from: 'user' });
}).toThrow();
});

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

expect(() => {
program.parse(['excess'], { from: 'user' });
}).not.toThrow();
});

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

expect(() => {
program.parse(['excess'], { from: 'user' });
}).not.toThrow();
});

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

expect(() => {
program.parse(['sub', 'excess'], { from: 'user' });
}).not.toThrow();
});

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

expect(() => {
program.parse(['sub', 'excess'], { from: 'user' });
}).toThrow();
});

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

expect(() => {
program.parse(['file'], { from: 'user' });
}).not.toThrow();
});

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

expect(() => {
program.parse(['file', 'excess'], { from: 'user' });
}).toThrow();
});

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

expect(() => {
program.parse(['file', 'excess'], { from: 'user' });
}).toThrow();
});

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

expect(() => {
program.parse(['file1', 'file2', 'file3'], { from: 'user' });
}).not.toThrow();
});
});
6 changes: 6 additions & 0 deletions tests/command.chain.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ describe('Command methods that should return this for chaining', () => {
expect(result).toBe(program);
});

test('when call .allowExcessArguments() then returns this', () => {
const program = new Command();
const result = program.allowExcessArguments();
expect(result).toBe(program);
});

test('when call .storeOptionsAsProperties() then returns this', () => {
const program = new Command();
const result = program.storeOptionsAsProperties();
Expand Down
37 changes: 37 additions & 0 deletions tests/command.exitOverride.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,43 @@ describe('.exitOverride and error details', () => {
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
.exitOverride()
.allowExcessArguments(false)
.action(() => { });

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

expect(stderrSpy).toHaveBeenCalled();
expectCommanderError(caughtErr, 1, 'commander.excessArguments', 'error: too many arguments. Expected 0 arguments but got 1.');
});

test('when specify command with excess argument then throw CommanderError', () => {
const program = new commander.Command();
program
.exitOverride()
.command('speak')
.allowExcessArguments(false)
.action(() => { });

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

expect(stderrSpy).toHaveBeenCalled();
expectCommanderError(caughtErr, 1, 'commander.excessArguments', "error: too many arguments for 'speak'. Expected 0 arguments but got 1.");
});

test('when specify --help then throw CommanderError', () => {
const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => { });
const program = new commander.Command();
Expand Down
4 changes: 4 additions & 0 deletions typings/commander-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@ const combineFlagAndOptionalValueThis2: commander.Command = program.combineFlagA
const allowUnknownOptionThis1: commander.Command = program.allowUnknownOption();
const allowUnknownOptionThis2: commander.Command = program.allowUnknownOption(false);

// allowExcessArguments
const allowExcessArgumentsThis1: commander.Command = program.allowExcessArguments();
const allowExcessArgumentsThis2: commander.Command = program.allowExcessArguments(false);

// parse
const parseThis1: commander.Command = program.parse();
const parseThis2: commander.Command = program.parse(process.argv);
Expand Down
7 changes: 7 additions & 0 deletions typings/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,13 @@ declare namespace commander {
*/
allowUnknownOption(allowUnknown?: boolean): this;

/**
* Allow excess arguments on the command line.
*
* @returns `this` command for chaining
*/
allowExcessArguments(allowExcess?: boolean): this;

/**
* Parse `argv`, setting options and invoking commands when defined.
*
Expand Down