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

Make command-argument array public (in readonly mode) #1970

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
15 changes: 15 additions & 0 deletions docs/deprecated.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ They are currently still available for backwards compatibility, but should not b
- [InvalidOptionArgumentError](#invalidoptionargumenterror)
- [Short option flag longer than a single character](#short-option-flag-longer-than-a-single-character)
- [Import from `commander/esm.mjs`](#import-from-commanderesmmjs)
- [.\_args](#_args)

## RegExp .option() parameter

Expand Down Expand Up @@ -207,3 +208,17 @@ import { Command } from 'commander';
```

README updated in Commander v9. Deprecated from Commander v9.

## ._args
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved

The registered command argument instances have been made accessible via `.registeredArguments`.

```js
const program = new Command().arguments('arg1 <arg2> [arg3]');
const argumentNamesForUsage = program.registeredArguments
.map(arg => arg.required ? `< ${arg.name()} >` : `[ ${arg.name()} ]`)
.join(' ');
program.usage('program ' + argumentNamesForUsage);
```

The older name of the property was `_args`. It was never documented but is still available so that old code relying on it keeps working. Do not use it.
31 changes: 16 additions & 15 deletions lib/command.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ class Command extends EventEmitter {
this._allowUnknownOption = false;
this._allowExcessArguments = true;
/** @type {Argument[]} */
this._args = [];
this.registeredArguments = [];
this._args = this.registeredArguments; // deprecated
/** @type {string[]} */
this.args = []; // cli args with options removed
this.rawArgs = [];
Expand Down Expand Up @@ -343,14 +344,14 @@ class Command extends EventEmitter {
* @return {Command} `this` command for chaining
*/
addArgument(argument) {
const previousArgument = this._args.slice(-1)[0];
const previousArgument = this.registeredArguments.slice(-1)[0];
if (previousArgument && previousArgument.variadic) {
throw new Error(`only the last argument can be variadic '${previousArgument.name()}'`);
}
if (argument.required && argument.defaultValue !== undefined && argument.parseArg === undefined) {
throw new Error(`a default value for a required argument is never used: '${argument.name()}'`);
}
this._args.push(argument);
this.registeredArguments.push(argument);
return this;
}

Expand Down Expand Up @@ -470,7 +471,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
action(fn) {
const listener = (args) => {
// The .action callback takes an extra parameter which is the command or options.
const expectedArgsCount = this._args.length;
const expectedArgsCount = this.registeredArguments.length;
const actionArgs = args.slice(0, expectedArgsCount);
if (this._storeOptionsAsProperties) {
actionArgs[expectedArgsCount] = this; // backwards compatible "options"
Expand Down Expand Up @@ -1105,29 +1106,29 @@ Expecting one of '${allowedValues.join("', '")}'`);
}

/**
* Check this.args against expected this._args.
* Check this.args against expected this.registeredArguments.
*
* @api private
*/

_checkNumberOfArguments() {
// too few
this._args.forEach((arg, i) => {
this.registeredArguments.forEach((arg, i) => {
if (arg.required && this.args[i] == null) {
this.missingArgument(arg.name());
}
});
// too many
if (this._args.length > 0 && this._args[this._args.length - 1].variadic) {
return;
}
if (this.args.length > this._args.length) {
if (this.registeredArguments.length > 0 &&
this.registeredArguments[this.registeredArguments.length - 1].variadic
) return;
if (this.args.length > this.registeredArguments.length) {
this._excessArguments(this.args);
}
}

/**
* Process this.args using this._args and save as this.processedArgs!
* Process this.args using this.registeredArguments and save as this.processedArgs!
*
* @api private
*/
Expand All @@ -1153,7 +1154,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
this._checkNumberOfArguments();

const processedArgs = [];
this._args.forEach((declaredArg, index) => {
this.registeredArguments.forEach((declaredArg, index) => {
let value = declaredArg.defaultValue;
if (declaredArg.variadic) {
// Collect together remaining arguments for passing together as an array.
Expand Down Expand Up @@ -1768,7 +1769,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
_excessArguments(receivedArgs) {
if (this._allowExcessArguments) return;

const expected = this._args.length;
const expected = this.registeredArguments.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}.`;
Expand Down Expand Up @@ -1909,13 +1910,13 @@ Expecting one of '${allowedValues.join("', '")}'`);
if (str === undefined) {
if (this._usage) return this._usage;

const args = this._args.map((arg) => {
const args = this.registeredArguments.map((arg) => {
return humanReadableArgName(arg);
});
return [].concat(
(this.options.length || this._hasHelpOption ? '[options]' : []),
(this.commands.length ? '[command]' : []),
(this._args.length ? args : [])
(this.registeredArguments.length ? args : [])
).join(' ');
}

Expand Down
8 changes: 4 additions & 4 deletions lib/help.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,14 @@ class Help {
visibleArguments(cmd) {
// Side effect! Apply the legacy descriptions before the arguments are displayed.
if (cmd._argsDescription) {
cmd._args.forEach(argument => {
cmd.registeredArguments.forEach(argument => {
argument.description = argument.description || cmd._argsDescription[argument.name()] || '';
});
}

// If there are any arguments with a description then return all the arguments.
if (cmd._args.find(argument => argument.description)) {
return cmd._args;
if (cmd.registeredArguments.find(argument => argument.description)) {
return cmd.registeredArguments;
}
return [];
}
Expand All @@ -142,7 +142,7 @@ class Help {

subcommandTerm(cmd) {
// Legacy. Ignores custom usage string, and nested commands.
const args = cmd._args.map(arg => humanReadableArgName(arg)).join(' ');
const args = cmd.registeredArguments.map(arg => humanReadableArgName(arg)).join(' ');
return cmd._name +
(cmd._aliases[0] ? '|' + cmd._aliases[0] : '') +
(cmd.options.length ? ' [options]' : '') + // simplistic check for non-help option
Expand Down
18 changes: 9 additions & 9 deletions tests/command.argumentVariations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const commander = require('../');
// and not exhaustively testing all methods elsewhere.

test.each(getSingleArgCases('<explicit-required>'))('when add "<arg>" using %s then argument required', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'explicit-required',
required: true,
Expand All @@ -15,7 +15,7 @@ test.each(getSingleArgCases('<explicit-required>'))('when add "<arg>" using %s t
});

test.each(getSingleArgCases('implicit-required'))('when add "arg" using %s then argument required', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'implicit-required',
required: true,
Expand All @@ -26,7 +26,7 @@ test.each(getSingleArgCases('implicit-required'))('when add "arg" using %s then
});

test.each(getSingleArgCases('[optional]'))('when add "[arg]" using %s then argument optional', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'optional',
required: false,
Expand All @@ -37,7 +37,7 @@ test.each(getSingleArgCases('[optional]'))('when add "[arg]" using %s then argum
});

test.each(getSingleArgCases('<explicit-required...>'))('when add "<arg...>" using %s then argument required and variadic', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'explicit-required',
required: true,
Expand All @@ -48,7 +48,7 @@ test.each(getSingleArgCases('<explicit-required...>'))('when add "<arg...>" usin
});

test.each(getSingleArgCases('implicit-required...'))('when add "arg..." using %s then argument required and variadic', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'implicit-required',
required: true,
Expand All @@ -59,7 +59,7 @@ test.each(getSingleArgCases('implicit-required...'))('when add "arg..." using %s
});

test.each(getSingleArgCases('[optional...]'))('when add "[arg...]" using %s then argument optional and variadic', (methodName, cmd) => {
const argument = cmd._args[0];
const argument = cmd.registeredArguments[0];
const expectedShape = {
_name: 'optional',
required: false,
Expand All @@ -79,8 +79,8 @@ function getSingleArgCases(arg) {
}

test.each(getMultipleArgCases('<first>', '[second]'))('when add two arguments using %s then two arguments', (methodName, cmd) => {
expect(cmd._args[0].name()).toEqual('first');
expect(cmd._args[1].name()).toEqual('second');
expect(cmd.registeredArguments[0].name()).toEqual('first');
expect(cmd.registeredArguments[1].name()).toEqual('second');
});

function getMultipleArgCases(arg1, arg2) {
Expand All @@ -99,6 +99,6 @@ test('when add arguments using multiple methods then all added', () => {
cmd.arguments('<arg3> <arg4>');
cmd.argument('<arg5>');
cmd.addArgument(new commander.Argument('arg6'));
const argNames = cmd._args.map(arg => arg.name());
const argNames = cmd.registeredArguments.map(arg => arg.name());
expect(argNames).toEqual(['arg1', 'arg2', 'arg3', 'arg4', 'arg5', 'arg6']);
});
12 changes: 6 additions & 6 deletions tests/command.createArgument.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,20 @@ class MyCommand extends commander.Command {
test('when override createArgument then used for argument()', () => {
const program = new MyCommand();
program.argument('<file>');
expect(program._args.length).toEqual(1);
expect(program._args[0].myProperty).toEqual('MyArgument');
expect(program.registeredArguments.length).toEqual(1);
expect(program.registeredArguments[0].myProperty).toEqual('MyArgument');
});

test('when override createArgument then used for arguments()', () => {
const program = new MyCommand();
program.arguments('<file>');
expect(program._args.length).toEqual(1);
expect(program._args[0].myProperty).toEqual('MyArgument');
expect(program.registeredArguments.length).toEqual(1);
expect(program.registeredArguments[0].myProperty).toEqual('MyArgument');
});

test('when override createArgument and createCommand then used for argument of command()', () => {
const program = new MyCommand();
const sub = program.command('sub <file>');
expect(sub._args.length).toEqual(1);
expect(sub._args[0].myProperty).toEqual('MyArgument');
expect(sub.registeredArguments.length).toEqual(1);
expect(sub.registeredArguments[0].myProperty).toEqual('MyArgument');
});
3 changes: 3 additions & 0 deletions typings/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ export class Command {
processedArgs: any[];
readonly commands: readonly Command[];
readonly options: readonly Option[];
readonly registeredArguments: readonly Argument[];
/** @deprecated Use `.registeredArguments` instead. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use same style as other deprecated messages

Suggested change
/** @deprecated Use `.registeredArguments` instead. */
/** @deprecated since v11, instead use `.registeredArguments` */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought about specifying the version, but decided against it because the property had never been supported officially.

readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors
shadowspawn marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
readonly _args: readonly Argument[]; // added here for strikethrough highlighting in editors
readonly _args: readonly Argument[];

parent: Command | null;

constructor(name?: string);
Expand Down