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

Clean up state from previous parse call when calling parse() / parseAsync() #1919

Closed
wants to merge 18 commits into from
Closed

Clean up state from previous parse call when calling parse() / parseAsync() #1919

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3c6ef71
Clean up state from previous parse call
aweebit Jul 28, 2023
b6d53b2
Improve persistent option value logic
aweebit Jul 28, 2023
437291f
Improve logic for option values supplied in user code
aweebit Jul 28, 2023
fbe911f
Remove public method functionality overlap
aweebit Jul 28, 2023
ebf672a
Do not clear persistent state
aweebit Jul 29, 2023
7fd0b22
Clarify that default option values are persistent
aweebit Jul 29, 2023
940b111
Fix user-supplied option value source type
aweebit Jul 29, 2023
52d5885
Use string instead of OptionValueSource
aweebit Jul 29, 2023
94e439d
Make user-supplied option values non-persistent while parsing
aweebit Jul 29, 2023
274be1a
Add non-persistent user-supplied option support for subcommands
aweebit Jul 29, 2023
56eed63
Fix conditional method call in setOptionValueWithSource()
aweebit Jul 30, 2023
4a7bc41
Reset subcommand parse state when dispatching
aweebit Jul 30, 2023
fcc97e2
Change OptionValueSource to string in type tests
aweebit Jul 30, 2023
36fa763
Return proxy redirecting keys for options-as-properties from Command …
aweebit Jul 30, 2023
829655d
Make comment about options with conflicting names more precise
aweebit Jul 30, 2023
633e4e4
Use right variable name
aweebit Jul 30, 2023
be00f59
Fully support options with names conflicting with instance properties
aweebit Jul 30, 2023
b53703b
Return proxy from constructor further up the prototype chain
aweebit Aug 3, 2023
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
249 changes: 157 additions & 92 deletions lib/command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,6 @@ class Command extends EventEmitter {
this._allowExcessArguments = true;
/** @type {Argument[]} */
this._args = [];
/** @type {string[]} */
this.args = []; // cli args with options removed
this.rawArgs = [];
this.processedArgs = []; // like .args but after custom processing and collecting variadic
this._scriptPath = null;
this._name = name || '';
this._optionValues = {};
this._optionValueSources = {}; // default, env, cli etc
this._storeOptionsAsProperties = false;
this._actionHandler = null;
this._executableHandler = false;
Expand Down Expand Up @@ -77,6 +69,28 @@ class Command extends EventEmitter {
this._helpCommandnameAndArgs = 'help [command]';
this._helpCommandDescription = 'display help for command';
this._helpConfiguration = {};

this._unprocessedName = name || '';
this._persistentOptionValues = {};
this._persistentOptionValueSources = {};
this.resetParseState();

/** @type {boolean | undefined} */
this._asyncParsing = undefined;
}

resetParseState() {
/** @type {string[]} */
this.args = []; // cli args with options removed
this.rawArgs = [];
this.processedArgs = []; // like .args but after custom processing and collecting variadic
this._scriptPath = null;

this._name = this._unprocessedName;
this._optionValues = Object.assign({}, this._persistentOptionValues);
this._optionValueSources = Object.assign(
{}, this._persistentOptionValueSources
); // default, env, cli etc
}

/**
Expand Down Expand Up @@ -515,10 +529,10 @@ Expecting one of '${allowedValues.join("', '")}'`);
// --no-foo is special and defaults foo to true, unless a --foo option is already defined
const positiveLongFlag = option.long.replace(/^--no-/, '--');
if (!this._findOption(positiveLongFlag)) {
this.setOptionValueWithSource(name, option.defaultValue === undefined ? true : option.defaultValue, 'default');
this._setPersistentOptionValueWithSource(name, option.defaultValue === undefined ? true : option.defaultValue, 'default');
}
} else if (option.defaultValue !== undefined) {
this.setOptionValueWithSource(name, option.defaultValue, 'default');
this._setPersistentOptionValueWithSource(name, option.defaultValue, 'default');
}

// register the option
Expand Down Expand Up @@ -558,7 +572,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
val = ''; // not normal, parseArg might have failed or be a mock function for testing
}
}
this.setOptionValueWithSource(name, val, valueSource);
this._setNonPersistentOptionValueWithSource(name, val, valueSource);
};

this.on('option:' + oname, (val) => {
Expand Down Expand Up @@ -780,34 +794,61 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

setOptionValue(key, value) {
return this.setOptionValueWithSource(key, value, undefined);
return this.setOptionValueWithSource(key, value);
}

/**
* Store option value and where the value came from.
*
* @param {string} key
* @param {Object} value
* @param {string} source - expected values are default/config/env/cli/implied
* @param {string} [source]
* @return {Command} `this` command for chaining
*/

setOptionValueWithSource(key, value, source) {
const set = this._asyncParsing === undefined
? this._setPersistentOptionValueWithSource
: this._setNonPersistentOptionValueWithSource;
set(key, value, source);
aweebit marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

/**
* @param {string} key
* @param {Object} value
* @param {string} [source]
* @api private
*/

_setPersistentOptionValueWithSource(key, value, source) {
this._setNonPersistentOptionValueWithSource(key, value, source);
this._persistentOptionValues[key] = value;
this._persistentOptionValueSources[key] = source;
}

/**
* @param {string} key
* @param {Object} value
* @param {string} [source]
* @api private
*/

_setNonPersistentOptionValueWithSource(key, value, source) {
if (this._storeOptionsAsProperties) {
this[key] = value;
} else {
this._optionValues[key] = value;
}
this._optionValueSources[key] = source;
return this;
}

/**
* Get source of option value.
* Expected values are default | config | env | cli | implied
*
* @param {string} key
* @return {string}
* @return {string | undefined}
*/

getOptionValueSource(key) {
Expand All @@ -819,7 +860,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
* Expected values are default | config | env | cli | implied
*
* @param {string} key
* @return {string}
* @return {string | undefined}
*/

getOptionValueSourceWithGlobals(key) {
Expand Down Expand Up @@ -887,6 +928,22 @@ Expecting one of '${allowedValues.join("', '")}'`);
return userArgs;
}

/**
* @param {boolean} async
* @param {Function} userArgsCallback
* @param {string[]} [argv]
* @param {Object} [parseOptions]
* @param {string} [parseOptions.from]
* @return {Command|Promise}
* @api private
*/

_parseSubroutine(async, userArgsCallback, argv, parseOptions) {
this.resetParseState();
const userArgs = this._prepareUserArgs(argv, parseOptions);
return userArgsCallback(userArgs);
}

/**
* Parse `argv`, setting options and invoking commands when defined.
*
Expand All @@ -905,10 +962,10 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

parse(argv, parseOptions) {
const userArgs = this._prepareUserArgs(argv, parseOptions);
this._parseCommand([], userArgs);

return this;
return this._parseSubroutine(false, (userArgs) => {
this._parseCommand([], userArgs);
return this;
}, argv, parseOptions);
}

/**
Expand All @@ -931,10 +988,10 @@ Expecting one of '${allowedValues.join("', '")}'`);
*/

async parseAsync(argv, parseOptions) {
const userArgs = this._prepareUserArgs(argv, parseOptions);
await this._parseCommand([], userArgs);

return this;
return this._parseSubroutine(true, async(userArgs) => {
await this._parseCommand([], userArgs);
return this;
}, argv, parseOptions);
}

/**
Expand Down Expand Up @@ -1078,7 +1135,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
if (subCommand._executableHandler) {
this._executeSubCommand(subCommand, operands.concat(unknown));
} else {
return subCommand._parseCommand(operands, unknown);
return subCommand._parseCommand(operands, unknown, this._asyncParsing);
}
});
return hookResult;
Expand Down Expand Up @@ -1256,81 +1313,87 @@ Expecting one of '${allowedValues.join("', '")}'`);
* @api private
*/

_parseCommand(operands, unknown) {
const parsed = this.parseOptions(unknown);
this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env
this._parseOptionsImplied();
operands = operands.concat(parsed.operands);
unknown = parsed.unknown;
this.args = operands.concat(unknown);

if (operands && this._findCommand(operands[0])) {
return this._dispatchSubcommand(operands[0], operands.slice(1), unknown);
}
if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) {
return this._dispatchHelpCommand(operands[1]);
}
if (this._defaultCommandName) {
outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command
return this._dispatchSubcommand(this._defaultCommandName, operands, unknown);
}
if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) {
// probably missing subcommand and no handler, user needs help (and exit)
this.help({ error: true });
}
_parseCommand(operands, unknown, async) {
this._asyncParsing = async;
Copy link
Collaborator

@shadowspawn shadowspawn Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is the _asyncParsing property bring used to track whether in the middle of a parse, rather than "async"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for motivation see commit message of 94e439d. #1917 does not use _asyncParsing anymore, but there would still be a nasty redundancy that I think would not even result in a merge conflict when #1915 is merged, which I do hope will happen eventually. Maybe it is not such a good idea to keep other PRs in mind, though. I am not sure since I have almost zero experience with contributing to open source.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is not such a good idea to keep other PRs in mind, though.

In my opinion it is ok to keep them in mind, and helpful to warn about potential merge issues.

(Even ok in my opinion to suggest it should land after another PR, but then be prepared for a rewrite if that other one gets rejected!)

Copy link
Contributor Author

@aweebit aweebit Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added #1915 as a peer PR, analogous to what I did for #1923 and #1924.


outputHelpIfRequested(this, parsed.unknown);
this._checkForMissingMandatoryOptions();
this._checkForConflictingOptions();
try {
const parsed = this.parseOptions(unknown);
this._parseOptionsEnv(); // after cli, so parseArg not called on both cli and env
this._parseOptionsImplied();
operands = operands.concat(parsed.operands);
unknown = parsed.unknown;
this.args = operands.concat(unknown);

// 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]);
if (operands && this._findCommand(operands[0])) {
return this._dispatchSubcommand(operands[0], operands.slice(1), unknown);
}
};

const commandEvent = `command:${this.name()}`;
if (this._actionHandler) {
checkForUnknownOptions();
this._processArguments();

let actionResult;
actionResult = this._chainOrCallHooks(actionResult, 'preAction');
actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs));
if (this.parent) {
actionResult = this._chainOrCall(actionResult, () => {
this.parent.emit(commandEvent, operands, unknown); // legacy
});
if (this._hasImplicitHelpCommand() && operands[0] === this._helpCommandName) {
return this._dispatchHelpCommand(operands[1]);
}
actionResult = this._chainOrCallHooks(actionResult, 'postAction');
return actionResult;
}
if (this.parent && this.parent.listenerCount(commandEvent)) {
checkForUnknownOptions();
this._processArguments();
this.parent.emit(commandEvent, operands, unknown); // legacy
} else if (operands.length) {
if (this._findCommand('*')) { // legacy default command
return this._dispatchSubcommand('*', operands, unknown);
if (this._defaultCommandName) {
outputHelpIfRequested(this, unknown); // Run the help for default command from parent rather than passing to default command
return this._dispatchSubcommand(this._defaultCommandName, operands, unknown);
}
if (this.listenerCount('command:*')) {
// skip option check, emit event for possible misspelling suggestion
this.emit('command:*', operands, unknown);
if (this.commands.length && this.args.length === 0 && !this._actionHandler && !this._defaultCommandName) {
// probably missing subcommand and no handler, user needs help (and exit)
this.help({ error: true });
}

outputHelpIfRequested(this, parsed.unknown);
this._checkForMissingMandatoryOptions();
this._checkForConflictingOptions();

// 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();
this._processArguments();

let actionResult;
actionResult = this._chainOrCallHooks(actionResult, 'preAction');
actionResult = this._chainOrCall(actionResult, () => this._actionHandler(this.processedArgs));
if (this.parent) {
actionResult = this._chainOrCall(actionResult, () => {
this.parent.emit(commandEvent, operands, unknown); // legacy
});
}
actionResult = this._chainOrCallHooks(actionResult, 'postAction');
return actionResult;
}
if (this.parent && this.parent.listenerCount(commandEvent)) {
checkForUnknownOptions();
this._processArguments();
this.parent.emit(commandEvent, operands, unknown); // legacy
} else if (operands.length) {
if (this._findCommand('*')) { // legacy default command
return this._dispatchSubcommand('*', operands, unknown);
}
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();
this._processArguments();
}
} else if (this.commands.length) {
this.unknownCommand();
checkForUnknownOptions();
// This command has subcommands and nothing hooked up at this level, so display help (and exit).
this.help({ error: true });
} else {
checkForUnknownOptions();
this._processArguments();
// fall through for caller to handle after calling .parse()
}
} else if (this.commands.length) {
checkForUnknownOptions();
// This command has subcommands and nothing hooked up at this level, so display help (and exit).
this.help({ error: true });
} else {
checkForUnknownOptions();
this._processArguments();
// fall through for caller to handle after calling .parse()
} finally {
this._asyncParsing = undefined;
}
}

Expand Down Expand Up @@ -1650,7 +1713,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
Object.keys(option.implied)
.filter(impliedKey => !hasCustomOptionValue(impliedKey))
.forEach(impliedKey => {
this.setOptionValueWithSource(impliedKey, option.implied[impliedKey], 'implied');
this._setNonPersistentOptionValueWithSource(
impliedKey, option.implied[impliedKey], 'implied'
);
});
});
}
Expand Down Expand Up @@ -1932,7 +1997,7 @@ Expecting one of '${allowedValues.join("', '")}'`);

name(str) {
if (str === undefined) return this._name;
this._name = str;
this._name = this._unprocessedName = str;
return this;
}

Expand Down
Loading