From 7daaa56d55905c6e6a27b10f0ca82e0b27c789cf Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 23 Oct 2023 12:48:10 -0700 Subject: [PATCH 01/18] Prevent ambiguous abbreviations of parameters defined on parent tool or action --- apps/heft/src/startWithVersionSelector.ts | 17 +- common/reviews/api/ts-command-line.api.md | 16 +- .../src/providers/AliasCommandLineAction.ts | 15 +- .../src/providers/CommandLineAction.ts | 16 + .../providers/CommandLineParameterProvider.ts | 89 ++-- .../src/providers/CommandLineParser.ts | 2 +- .../src/providers/ScopedCommandLineAction.ts | 34 +- .../test/AmbiguousCommandLineParser.test.ts | 397 +++++++++++++++++- .../AmbiguousCommandLineParser.test.ts.snap | 102 ++++- .../CommandLineParameter.test.ts.snap | 2 - .../ScopedCommandLineAction.test.ts.snap | 7 +- 11 files changed, 648 insertions(+), 49 deletions(-) diff --git a/apps/heft/src/startWithVersionSelector.ts b/apps/heft/src/startWithVersionSelector.ts index 1b13a03e0f6..160d7671c16 100644 --- a/apps/heft/src/startWithVersionSelector.ts +++ b/apps/heft/src/startWithVersionSelector.ts @@ -49,14 +49,27 @@ function tryGetPackageFolderFor(resolvedFileOrFolderPath: string): string | unde * Use "heft --unmanaged" to bypass this feature. */ function tryStartLocalHeft(): boolean { - if (process.argv.indexOf(Constants.unmanagedParameterLongName) >= 0) { + const toolArgs: Set = new Set(); + // Skip the first two arguments, which are the path to the Node executable and the path to the Heft + // entrypoint. The remaining arguments are the tool arguments. Grab them until we reach a non-"--"-prefixed + // argument. We can do this simple parsing because the Heft tool only has simple optional flags. + for (let i: number = 2; i < process.argv.length; ++i) { + const arg: string = process.argv[i]; + if (!arg.startsWith('--')) { + break; + } + + toolArgs.add(arg); + } + + if (toolArgs.has(Constants.unmanagedParameterLongName)) { console.log( `Bypassing the Heft version selector because ${JSON.stringify(Constants.unmanagedParameterLongName)} ` + 'was specified.' ); console.log(); return false; - } else if (process.argv.indexOf(Constants.debugParameterLongName) >= 0) { + } else if (toolArgs.has(Constants.debugParameterLongName)) { // The unmanaged flag could be undiscoverable if it's not in their locally installed version console.log( 'Searching for a locally installed version of Heft. Use the ' + diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index c02e5de44d0..6a684ec766f 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -14,7 +14,7 @@ export class AliasCommandLineAction extends CommandLineAction { // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; // @internal (undocumented) - _registerDefinedParameters(): void; + _registerDefinedParameters(existingParameterNames: Set): void; readonly targetAction: CommandLineAction; } @@ -158,6 +158,10 @@ export enum CommandLineParameterKind { export abstract class CommandLineParameterProvider { // @internal constructor(); + // @internal (undocumented) + readonly _ambiguousParameterParserKeys: Map; + // @internal (undocumented) + protected _defineAmbiguousParameter(name: string): string; defineChoiceListParameter(definition: ICommandLineChoiceListDefinition): CommandLineChoiceListParameter; defineChoiceParameter(definition: ICommandLineChoiceDefinition): CommandLineChoiceParameter; defineCommandLineRemainder(definition: ICommandLineRemainderDefinition): CommandLineRemainder; @@ -181,11 +185,17 @@ export abstract class CommandLineParameterProvider { protected onDefineParameters?(): void; get parameters(): ReadonlyArray; get parametersProcessed(): boolean; + // @internal (undocumented) + protected _parametersRegistered: boolean; parseScopedLongName(scopedLongName: string): IScopedLongNameParseResult; // @internal (undocumented) protected _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; + // (undocumented) + protected _registerAmbiguousParameter(name: string, parserKey: string): void; // @internal (undocumented) - _registerDefinedParameters(): void; + _registerDefinedParameters(existingParameterNames?: Set): void; + // @internal (undocumented) + protected readonly _registeredParameterNames: Set; // @internal (undocumented) protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean): void; get remainder(): CommandLineRemainder | undefined; @@ -382,6 +392,8 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { get parameters(): ReadonlyArray; // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; + // @internal (undocumented) + _registerDefinedParameters(existingParameterNames?: Set): void; static readonly ScopingParameterGroup: typeof SCOPING_PARAMETER_GROUP; } diff --git a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts index 9f6e19c4813..8832809d15d 100644 --- a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts @@ -85,8 +85,13 @@ export class AliasCommandLineAction extends CommandLineAction { } /** @internal */ - public _registerDefinedParameters(): void { + public _registerDefinedParameters(existingParameterNames?: Set): void { /* override */ + // First, we need to make sure that the target action is registered, since we need to re-use + // its parameters, and some of these only get defined during registration. This will no-op if + // the target action is already registered. + this.targetAction._registerDefinedParameters(existingParameterNames); + // All parameters are going to be defined by the target action. Re-use the target action parameters // for this action. for (const parameter of this.targetAction.parameters) { @@ -142,6 +147,12 @@ export class AliasCommandLineAction extends CommandLineAction { this._parameterKeyMap.set(aliasParameter._parserKey!, parameter._parserKey!); } + // Also re-use the target action ambiguous parameters for this action + for (const [ambiguousParameterName, parserKey] of this.targetAction._ambiguousParameterParserKeys) { + const aliasParserKey: string = this._defineAmbiguousParameter(ambiguousParameterName); + this._parameterKeyMap.set(aliasParserKey, parserKey); + } + // We also need to register the remainder parameter if the target action has one. The parser // key for this parameter is constant. if (this.targetAction.remainder) { @@ -150,7 +161,7 @@ export class AliasCommandLineAction extends CommandLineAction { } // Finally, register the parameters with the parser. - super._registerDefinedParameters(); + super._registerDefinedParameters(existingParameterNames); } /** diff --git a/libraries/ts-command-line/src/providers/CommandLineAction.ts b/libraries/ts-command-line/src/providers/CommandLineAction.ts index 9c65acf8b3e..7fba7abfcf7 100644 --- a/libraries/ts-command-line/src/providers/CommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/CommandLineAction.ts @@ -5,6 +5,7 @@ import type * as argparse from 'argparse'; import { CommandLineParameterProvider, type ICommandLineParserData } from './CommandLineParameterProvider'; import type { ICommandLineParserOptions } from './CommandLineParser'; +import { CommandLineParserExitError } from './CommandLineParserExitError'; /** * Options for the CommandLineAction constructor. @@ -87,6 +88,21 @@ export abstract class CommandLineAction extends CommandLineParameterProvider { description: this.documentation }); + // Monkey-patch the error handling for the action parser + this._argumentParser.exit = (status: number, message: string) => { + throw new CommandLineParserExitError(status, message); + }; + const originalArgumentParserErrorFn: (err: Error | string) => void = this._argumentParser.error.bind( + this._argumentParser + ); + this._argumentParser.error = (err: Error | string) => { + // Ensure the ParserExitError bubbles up to the top without any special processing + if (err instanceof CommandLineParserExitError) { + throw err; + } + originalArgumentParserErrorFn(err); + }; + this.onDefineParameters?.(); } diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index ad1cd6bbf1c..3713be958d5 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -28,6 +28,7 @@ import { CommandLineStringParameter } from '../parameters/CommandLineStringParam import { CommandLineStringListParameter } from '../parameters/CommandLineStringListParameter'; import { CommandLineRemainder } from '../parameters/CommandLineRemainder'; import { SCOPING_PARAMETER_GROUP } from '../Constants'; +import { CommandLineParserExitError } from './CommandLineParserExitError'; /** * The result containing the parsed paramter long name and scope. Returned when calling @@ -73,6 +74,13 @@ const POSSIBLY_SCOPED_LONG_NAME_REGEXP: RegExp = export abstract class CommandLineParameterProvider { private static _keyCounter: number = 0; + /** @internal */ + public readonly _ambiguousParameterParserKeys: Map; + /** @internal */ + protected readonly _registeredParameterNames: Set = new Set(); + /** @internal */ + protected _parametersRegistered: boolean; + private readonly _parameters: CommandLineParameter[]; private readonly _parametersByLongName: Map; private readonly _parametersByShortName: Map; @@ -80,8 +88,6 @@ export abstract class CommandLineParameterProvider { string | typeof SCOPING_PARAMETER_GROUP, argparse.ArgumentGroup >; - private readonly _ambiguousParameterNamesByParserKey: Map; - private _parametersRegistered: boolean; private _parametersProcessed: boolean; private _remainder: CommandLineRemainder | undefined; @@ -92,7 +98,7 @@ export abstract class CommandLineParameterProvider { this._parametersByLongName = new Map(); this._parametersByShortName = new Map(); this._parameterGroupsByName = new Map(); - this._ambiguousParameterNamesByParserKey = new Map(); + this._ambiguousParameterParserKeys = new Map(); this._parametersRegistered = false; this._parametersProcessed = false; } @@ -405,15 +411,18 @@ export abstract class CommandLineParameterProvider { } /** @internal */ - public _registerDefinedParameters(): void { + public _registerDefinedParameters(existingParameterNames?: Set): void { if (this._parametersRegistered) { // We prevent new parameters from being defined after the first call to _registerDefinedParameters, // so we can already ensure that all parameters were registered. return; } - this._parametersRegistered = true; - const ambiguousParameterNames: Set = new Set(); + // Register the existing parameters as ambiguous parameters first. These are generally provided by the + // parent action. Register these first so that we can report matching parameters as errors. + for (const existingParameterName of existingParameterNames || []) { + this._defineAmbiguousParameter(existingParameterName); + } // First, loop through all parameters with short names. If there are any duplicates, disable the short names // since we can't prefix scopes to short names in order to deduplicate them. The duplicate short names will @@ -421,7 +430,7 @@ export abstract class CommandLineParameterProvider { for (const [shortName, shortNameParameters] of this._parametersByShortName.entries()) { if (shortNameParameters.length > 1) { for (const parameter of shortNameParameters) { - ambiguousParameterNames.add(shortName); + this._defineAmbiguousParameter(shortName); parameter._disableShortName(); } } @@ -440,16 +449,16 @@ export abstract class CommandLineParameterProvider { 'Parameters with the same long name must define a scope.' ); } - ambiguousParameterNames.add(parameter.longName); + this._defineAmbiguousParameter(parameter.longName); } this._registerParameter(parameter, useScopedLongName); } } - // Register silent parameters for the ambiguous short names and long names to ensure that users are made - // aware that the provided argument is ambiguous. - for (const ambiguousParameterName of ambiguousParameterNames) { - this._registerAmbiguousParameter(ambiguousParameterName); + // We also need to loop through the defined ambiguous parameters and register them. These will be reported + // as errors if the user attempts to use them. + for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeys) { + this._registerAmbiguousParameter(ambiguousParameterName, parserKey); } // Need to add the remainder parameter last @@ -462,6 +471,8 @@ export abstract class CommandLineParameterProvider { this._getArgumentParser().addArgument(argparse.Const.REMAINDER, argparseOptions); } + + this._parametersRegistered = true; } /** @@ -487,8 +498,14 @@ export abstract class CommandLineParameterProvider { } // Search for any ambiguous parameters and throw an error if any are found - for (const [parserKey, parameterName] of this._ambiguousParameterNamesByParserKey) { + for (const [parameterName, parserKey] of this._ambiguousParameterParserKeys) { if (data[parserKey]) { + // Write out the usage text to make it easier for the user to find the correct parameter name + const errorPrefix: string = `${this.renderUsageText()}\n${parserOptions.toolFilename} ${ + data.aliasAction || data.action + }: error: `; + const errorPostfix: string = '\n'; + // Determine if the ambiguous parameter is a short name or a long name, since the process of finding // the non-ambiguous name is different for each. const duplicateShortNameParameters: CommandLineParameter[] | undefined = @@ -525,9 +542,10 @@ export abstract class CommandLineParameterProvider { // short name, ex. // Error: The short parameter name "-p" is ambiguous. It could refer to any of the following // parameters: "--param1", "--param2" - throw new Error( - `The short parameter name "${parameterName}" is ambiguous. It could refer to any of ` + - `the following parameters: "${nonAmbiguousLongNames.join('", "')}"` + throw new CommandLineParserExitError( + 1, + `${errorPrefix}The short parameter name "${parameterName}" is ambiguous. It could refer to any of ` + + `the following parameters: "${nonAmbiguousLongNames.join('", "')}"${errorPostfix}` ); } @@ -551,14 +569,17 @@ export abstract class CommandLineParameterProvider { // ambiguous long name, ex. // Error: The parameter name "--param" is ambiguous. It could refer to any of the following // parameters: "--scope1:param", "--scope2:param" - throw new Error( - `The parameter name "${parameterName}" is ambiguous. It could refer to any of ` + - `the following parameters: "${nonAmbiguousLongNames.join('", "')}"` + throw new CommandLineParserExitError( + 1, + `${errorPrefix}The parameter name "${parameterName}" is ambiguous. It could refer to any of ` + + `the following parameters: "${nonAmbiguousLongNames.join('", "')}"${errorPostfix}` ); } - // This shouldn't happen, but we also shouldn't allow the user to use the ambiguous parameter - throw new Error(`The parameter name "${parameterName}" is ambiguous.`); + throw new CommandLineParserExitError( + 1, + `${errorPrefix}The parameter name "${parameterName}" is ambiguous.${errorPostfix}` + ); } } @@ -609,6 +630,22 @@ export abstract class CommandLineParameterProvider { } } + /** @internal */ + protected _defineAmbiguousParameter(name: string): string { + if (this._parametersRegistered) { + throw new Error('Parameters have already been registered for this provider'); + } + + // Generate and set the parser key at definition time. Only generate a new parser key + // if the ambiguous paramter hasn't been defined yet. + let existingParserKey: string | undefined = this._ambiguousParameterParserKeys.get(name); + if (!existingParserKey) { + existingParserKey = this._generateKey(); + this._ambiguousParameterParserKeys.set(name, existingParserKey); + } + return existingParserKey; + } + /** @internal */ protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean): void { const names: string[] = []; @@ -707,12 +744,14 @@ export abstract class CommandLineParameterProvider { help: argparse.Const.SUPPRESS }); } - } - private _registerAmbiguousParameter(name: string): void { - const parserKey: string = this._generateKey(); - this._ambiguousParameterNamesByParserKey.set(parserKey, name); + // Register the parameter names so that we can detect ambiguous parameters + for (const name of [...names, ...(parameter.undocumentedSynonyms || [])]) { + this._registeredParameterNames.add(name); + } + } + protected _registerAmbiguousParameter(name: string, parserKey: string): void { this._getArgumentParser().addArgument(name, { dest: parserKey, // We don't know if this argument takes parameters or not, so we need to accept any number of args diff --git a/libraries/ts-command-line/src/providers/CommandLineParser.ts b/libraries/ts-command-line/src/providers/CommandLineParser.ts index 25d98f13199..20556981c2f 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParser.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParser.ts @@ -265,7 +265,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { public _registerDefinedParameters(): void { super._registerDefinedParameters(); for (const action of this._actions) { - action._registerDefinedParameters(); + action._registerDefinedParameters(this._registeredParameterNames); } } diff --git a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts index 6f349868231..b3715f89eb1 100644 --- a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts @@ -14,6 +14,7 @@ interface IInternalScopedCommandLineParserOptions extends ICommandLineParserOpti readonly onDefineScopedParameters: (commandLineParameterProvider: CommandLineParameterProvider) => void; readonly aliasAction?: string; readonly aliasDocumentation?: string; + readonly existingParameterNames?: Set; } /** @@ -58,6 +59,17 @@ class InternalScopedCommandLineParser extends CommandLineParser { this._internalOptions.onDefineScopedParameters(this); } + public _registerDefinedParameters(): void { + if (!this._parametersRegistered) { + // Manually register our ambiguous parameters from the parent tool and action + for (const existingParameterName of this._internalOptions.existingParameterNames || []) { + this._defineAmbiguousParameter(existingParameterName); + } + } + + super._registerDefinedParameters(); + } + protected async onExecute(): Promise { // override // Only set if we made it this far, which may not be the case if an error occurred or @@ -92,6 +104,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { private _scopingParameters: CommandLineParameter[]; private _unscopedParserOptions: ICommandLineParserOptions | undefined; private _scopedCommandLineParser: InternalScopedCommandLineParser | undefined; + private _existingParameterNames: Set = new Set(); /** * The required group name to apply to all scoping parameters. At least one parameter @@ -135,6 +148,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { aliasAction: data.aliasAction, aliasDocumentation: data.aliasDocumentation, unscopedActionParameters: this.parameters, + existingParameterNames: this._existingParameterNames, onDefineScopedParameters: this.onDefineScopedParameters.bind(this) }); } @@ -157,15 +171,12 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { const scopedArgs: string[] = []; if (this.remainder.values.length) { if (this.remainder.values[0] !== '--') { - // Imitate argparse behavior and log out usage text before throwing. - // eslint-disable-next-line no-console - console.log(this.renderUsageText()); throw new CommandLineParserExitError( // argparse sets exit code 2 for invalid arguments 2, // model the message off of the built-in "unrecognized arguments" message - `${this._unscopedParserOptions.toolFilename} ${this.actionName}: error: Unrecognized ` + - `arguments: ${this.remainder.values[0]}.` + `${this.renderUsageText()}\n${this._unscopedParserOptions.toolFilename} ${this.actionName}: ` + + `error: Unrecognized arguments: ${this.remainder.values[0]}.\n` ); } for (const scopedArg of this.remainder.values.slice(1)) { @@ -185,6 +196,19 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { return; } + /** @internal */ + public _registerDefinedParameters(existingParameterNames?: Set): void { + super._registerDefinedParameters(existingParameterNames); + + for (const registeredParameterName of this._registeredParameterNames) { + this._existingParameterNames.add(registeredParameterName); + } + + for (const existingParameterName of existingParameterNames || []) { + this._existingParameterNames.add(existingParameterName); + } + } + /** * {@inheritdoc CommandLineParameterProvider.onDefineParameters} */ diff --git a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts index 265716b863c..df2950be607 100644 --- a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts +++ b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts @@ -1,22 +1,24 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. -import { CommandLineAction } from '../providers/CommandLineAction'; -import type { CommandLineStringParameter } from '../parameters/CommandLineStringParameter'; import { CommandLineParser } from '../providers/CommandLineParser'; +import { CommandLineAction } from '../providers/CommandLineAction'; +import { AliasCommandLineAction } from '../providers/AliasCommandLineAction'; import { ScopedCommandLineAction } from '../providers/ScopedCommandLineAction'; +import type { CommandLineStringParameter } from '../parameters/CommandLineStringParameter'; import type { CommandLineFlagParameter } from '../parameters/CommandLineFlagParameter'; import type { CommandLineParameterProvider } from '../providers/CommandLineParameterProvider'; import { SCOPING_PARAMETER_GROUP } from '../Constants'; class GenericCommandLine extends CommandLineParser { - public constructor(actionType: new () => CommandLineAction) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + public constructor(actionType: new (...args: any[]) => CommandLineAction, ...args: any[]) { super({ toolFilename: 'example', toolDescription: 'An example project' }); - this.addAction(new actionType()); + this.addAction(new actionType(...args)); } } @@ -77,6 +79,38 @@ class AmbiguousAction extends CommandLineAction { } } +class AbbreviationAction extends CommandLineAction { + public done: boolean = false; + public abbreviationFlag: CommandLineFlagParameter; + + public constructor() { + super({ + actionName: 'do:the-job', + summary: 'does the job', + documentation: 'a longer description' + }); + + this.abbreviationFlag = this.defineFlagParameter({ + parameterLongName: '--abbreviation-flag', + description: 'The argument' + }); + } + + protected async onExecute(): Promise { + this.done = true; + } +} + +class AliasAction extends AliasCommandLineAction { + public constructor(targetActionClass: new () => CommandLineAction) { + super({ + toolFilename: 'example', + aliasName: 'do:the-job-alias', + targetAction: new targetActionClass() + }); + } +} + class AmbiguousScopedAction extends ScopedCommandLineAction { public done: boolean = false; public short1Value: string | undefined; @@ -165,6 +199,60 @@ class AmbiguousScopedAction extends ScopedCommandLineAction { } } +interface IAbbreviationScopedActionOptions { + includeUnscopedAbbreviationFlag: boolean; + includeScopedAbbreviationFlag: boolean; +} + +class AbbreviationScopedAction extends ScopedCommandLineAction { + public done: boolean = false; + public unscopedAbbreviationFlag: CommandLineFlagParameter | undefined; + public scopedAbbreviationFlag: CommandLineFlagParameter | undefined; + + private _scopingArg: CommandLineFlagParameter | undefined; + private _includeScopedAbbreviationFlag: boolean; + + public constructor(options: IAbbreviationScopedActionOptions) { + super({ + actionName: 'scoped-action', + summary: 'does the scoped action', + documentation: 'a longer description' + }); + + if (options?.includeUnscopedAbbreviationFlag) { + this.unscopedAbbreviationFlag = this.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + } + + this._includeScopedAbbreviationFlag = !!options?.includeScopedAbbreviationFlag; + } + + protected async onExecute(): Promise { + expect(this._scopingArg?.value).toEqual(true); + this.done = true; + } + + protected onDefineUnscopedParameters(): void { + // At least one scoping parameter is required to be defined on a scoped action + this._scopingArg = this.defineFlagParameter({ + parameterLongName: '--scoping', + description: 'The scoping parameter', + parameterGroup: SCOPING_PARAMETER_GROUP + }); + } + + protected onDefineScopedParameters(scopedParameterProvider: CommandLineParameterProvider): void { + if (this._includeScopedAbbreviationFlag) { + this.scopedAbbreviationFlag = scopedParameterProvider.defineFlagParameter({ + parameterLongName: '--abbreviation-flag', + description: 'A flag used to test abbreviation logic' + }); + } + } +} + describe(`Ambiguous ${CommandLineParser.name}`, () => { it('fails to execute when an ambiguous short name is provided', async () => { const commandLineParser: GenericCommandLine = new GenericCommandLine(AmbiguousAction); @@ -207,6 +295,181 @@ describe(`Ambiguous ${CommandLineParser.name}`, () => { commandLineParser.executeWithoutErrorHandling(['do:the-job', '--arg', 'test']) ).rejects.toThrowErrorMatchingSnapshot(); }); + + it('fails when an action declares a flag that was declared in the tool', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AbbreviationAction); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation-flag', + description: 'A flag used to test abbreviation logic' + }); + + expect(commandLineParser._registerDefinedParameters.bind(commandLineParser)).toThrowError( + /Conflicting option string\(s\): --abbreviation-flag/ + ); + }); + + it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AbbreviationAction); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbreviation']) + ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + }); + + it('fails when providing an ambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AbbreviationAction); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbrev']) + ).rejects.toThrowError(/Ambiguous option: "--abbrev" could match --abbreviation-flag, --abbreviation/); + }); + + it('allows unambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AbbreviationAction); + const toolAbbreviationFlag: CommandLineFlagParameter = commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbreviation-f']); + + expect(commandLineParser.selectedAction).toBeDefined(); + expect(commandLineParser.selectedAction!.actionName).toEqual('do:the-job'); + + const action: AbbreviationAction = commandLineParser.selectedAction as AbbreviationAction; + expect(action.done).toBe(true); + expect(action.abbreviationFlag.value).toBe(true); + expect(toolAbbreviationFlag.value).toBe(false); + }); +}); + +describe(`Ambiguous aliased ${CommandLineParser.name}`, () => { + it('fails to execute when an ambiguous short name is provided', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AmbiguousAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '-s']) + ).rejects.toThrowErrorMatchingSnapshot(); + }); + + it('can execute the non-ambiguous scoped long names', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AmbiguousAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + + await commandLineParser.execute([ + 'do:the-job-alias', + '--short1', + 'short1value', + '--short2', + 'short2value', + '--scope1:arg', + 'scope1value', + '--scope2:arg', + 'scope2value', + '--non-conflicting-arg', + 'nonconflictingvalue' + ]); + expect(commandLineParser.selectedAction).toBeDefined(); + expect(commandLineParser.selectedAction!.actionName).toEqual('do:the-job-alias'); + + const action: AmbiguousAction = (commandLineParser.selectedAction as AliasAction) + .targetAction as AmbiguousAction; + expect(action.done).toBe(true); + + expect(action.renderHelpText()).toMatchSnapshot(); + expect(action.getParameterStringMap()).toMatchSnapshot(); + }); + + it('fails to execute when an ambiguous long name is provided', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AmbiguousAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--arg', 'test']) + ).rejects.toThrowErrorMatchingSnapshot(); + }); + + it('fails when an action declares a flag that was declared in the tool', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AbbreviationAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation-flag', + description: 'A flag used to test abbreviation logic' + }); + + expect(commandLineParser._registerDefinedParameters.bind(commandLineParser)).toThrowError( + /Conflicting option string\(s\): --abbreviation-flag/ + ); + }); + + it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AbbreviationAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbreviation']) + ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + }); + + it('fails when providing an ambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AbbreviationAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbrev']) + ).rejects.toThrowError(/Ambiguous option: "--abbrev" could match --abbreviation-flag, --abbreviation/); + }); + + it('allows unambiguous abbreviation between flags on the tool and the action', async () => { + const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AbbreviationAction); + commandLineParser.addAction( + (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction + ); + const toolAbbreviationFlag: CommandLineFlagParameter = commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbreviation-f']); + + expect(commandLineParser.selectedAction).toBeDefined(); + expect(commandLineParser.selectedAction!.actionName).toEqual('do:the-job-alias'); + + const action: AbbreviationAction = (commandLineParser.selectedAction as AliasAction) + .targetAction as AbbreviationAction; + expect(action.done).toBe(true); + expect(action.abbreviationFlag.value).toBe(true); + expect(toolAbbreviationFlag.value).toBe(false); + }); }); describe(`Ambiguous scoping ${CommandLineParser.name}`, () => { @@ -263,4 +526,130 @@ describe(`Ambiguous scoping ${CommandLineParser.name}`, () => { commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--arg', 'test']) ).rejects.toThrowErrorMatchingSnapshot(); }); + + it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the scoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: false, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbreviation']) + ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + }); + + it('fails when providing an exact match to an ambiguous abbreviation between flags on the scoped action and the unscoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: true, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + + await expect( + commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbreviation']) + ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + }); + + it('fails when providing an ambiguous abbreviation between flags on the tool and the scoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: false, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + + await expect( + commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbrev']) + ).rejects.toThrowError(/Ambiguous option: "--abbrev" could match --abbreviation-flag, --abbreviation/); + }); + + it('fails when providing an ambiguous abbreviation between flags on the unscoped action and the scoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: true, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + + await expect( + commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbrev']) + ).rejects.toThrowError(/Ambiguous option: "--abbrev" could match --abbreviation-flag, --abbreviation/); + }); + + it('allows unambiguous abbreviation between flags on the tool and the scoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: false, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + const toolAbbreviationFlag: CommandLineFlagParameter = commandLineParser.defineFlagParameter({ + parameterLongName: '--abbreviation', + description: 'A flag used to test abbreviation logic' + }); + const targetAction: AbbreviationScopedAction = commandLineParser.getAction( + 'scoped-action' + ) as AbbreviationScopedAction; + + await commandLineParser.executeWithoutErrorHandling([ + 'scoped-action', + '--scoping', + '--', + '--abbreviation-f' + ]); + + expect(commandLineParser.selectedAction).toBeDefined(); + expect(commandLineParser.selectedAction!.actionName).toEqual('scoped-action'); + expect(targetAction.done).toBe(true); + expect(targetAction.scopedAbbreviationFlag?.value).toBe(true); + expect(toolAbbreviationFlag.value).toBe(false); + }); + + it('allows unambiguous abbreviation between flags on the unscoped action and the scoped action', async () => { + const actionOptions: IAbbreviationScopedActionOptions = { + includeUnscopedAbbreviationFlag: true, + includeScopedAbbreviationFlag: true + }; + const commandLineParser: GenericCommandLine = new GenericCommandLine( + AbbreviationScopedAction, + actionOptions + ); + const targetAction: AbbreviationScopedAction = commandLineParser.getAction( + 'scoped-action' + ) as AbbreviationScopedAction; + + await commandLineParser.executeWithoutErrorHandling([ + 'scoped-action', + '--scoping', + '--', + '--abbreviation-f' + ]); + + expect(commandLineParser.selectedAction).toBeDefined(); + expect(commandLineParser.selectedAction!.actionName).toEqual('scoped-action'); + expect(targetAction.done).toBe(true); + expect(targetAction.scopedAbbreviationFlag?.value).toBe(true); + expect(targetAction.unscopedAbbreviationFlag?.value).toBe(false); + }); }); diff --git a/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap b/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap index 8580473502e..5ab8d467679 100644 --- a/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap +++ b/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap @@ -29,12 +29,104 @@ Object { } `; -exports[`Ambiguous CommandLineParser fails to execute when an ambiguous long name is provided 1`] = `"The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\""`; +exports[`Ambiguous CommandLineParser fails to execute when an ambiguous long name is provided 1`] = ` +"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example do:the-job: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +" +`; + +exports[`Ambiguous CommandLineParser fails to execute when an ambiguous short name is provided 1`] = ` +"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example do:the-job: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +" +`; + +exports[`Ambiguous aliased CommandLineParser can execute the non-ambiguous scoped long names 1`] = ` +"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +a longer description + +Optional arguments: + -h, --help Show this help message and exit. + --short1 ARG The argument + --short2 ARG The argument + --scope1:arg ARG The argument + --scope2:arg ARG The argument + --non-conflicting-arg ARG, --scope:non-conflicting-arg ARG + The argument +" +`; -exports[`Ambiguous CommandLineParser fails to execute when an ambiguous short name is provided 1`] = `"The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\""`; +exports[`Ambiguous aliased CommandLineParser can execute the non-ambiguous scoped long names 2`] = ` +Object { + "--scope1:arg": "\\"scope1value\\"", + "--scope2:arg": "\\"scope2value\\"", + "--scope:non-conflicting-arg": "\\"nonconflictingvalue\\"", + "--short1": "\\"short1value\\"", + "--short2": "\\"short2value\\"", +} +`; -exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous long name is provided to a scoping action 1`] = `"The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\""`; +exports[`Ambiguous aliased CommandLineParser fails to execute when an ambiguous long name is provided 1`] = ` +"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] [--scope2:arg ARG] + [--non-conflicting-arg ARG] + -exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action 1`] = `"The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\""`; +example do:the-job-alias: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +" +`; -exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action with a matching ambiguous long name 1`] = `"The short parameter name \\"-a\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\", \\"--non-conflicting-arg\\""`; +exports[`Ambiguous aliased CommandLineParser fails to execute when an ambiguous short name is provided 1`] = ` +"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example do:the-job-alias: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +" +`; + +exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous long name is provided to a scoping action 1`] = ` +"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] + [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example scoped-action --scoping -- undefined: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +" +`; + +exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action 1`] = ` +"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] + [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example scoped-action --scoping -- undefined: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +" +`; + +exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action with a matching ambiguous long name 1`] = ` +"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] + [--scope1:arg ARG] + [--scope2:arg ARG] + [--non-conflicting-arg ARG] + + +example scoped-action --scoping -- undefined: error: The short parameter name \\"-a\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\", \\"--non-conflicting-arg\\" +" +`; diff --git a/libraries/ts-command-line/src/test/__snapshots__/CommandLineParameter.test.ts.snap b/libraries/ts-command-line/src/test/__snapshots__/CommandLineParameter.test.ts.snap index ed13c337e1d..08ae67d14a0 100644 --- a/libraries/ts-command-line/src/test/__snapshots__/CommandLineParameter.test.ts.snap +++ b/libraries/ts-command-line/src/test/__snapshots__/CommandLineParameter.test.ts.snap @@ -50,8 +50,6 @@ exports[`CommandLineParameter choice list raises an error if env var value is js exports[`CommandLineParameter choice list raises an error if env var value is not a valid choice 1`] = `"Invalid value \\"oblong\\" for the environment variable ENV_COLOR. Valid choices are: \\"purple\\", \\"yellow\\", \\"pizza\\""`; -exports[`CommandLineParameter choice list raises an error if env var value is not valid json 1`] = `"The [u environment variable value looks like a JSON array but failed to parse: Unexpected token u in JSON at position 1"`; - exports[`CommandLineParameter parses an input with ALL parameters 1`] = ` Object { "argumentName": undefined, diff --git a/libraries/ts-command-line/src/test/__snapshots__/ScopedCommandLineAction.test.ts.snap b/libraries/ts-command-line/src/test/__snapshots__/ScopedCommandLineAction.test.ts.snap index ca696e05d3d..0c256de22c8 100644 --- a/libraries/ts-command-line/src/test/__snapshots__/ScopedCommandLineAction.test.ts.snap +++ b/libraries/ts-command-line/src/test/__snapshots__/ScopedCommandLineAction.test.ts.snap @@ -56,7 +56,12 @@ Object { } `; -exports[`CommandLineParser throws on missing positional arg divider with unknown positional args 1`] = `"example scoped-action: error: Unrecognized arguments: bar."`; +exports[`CommandLineParser throws on missing positional arg divider with unknown positional args 1`] = ` +"usage: example scoped-action [-h] [--verbose] [--scope SCOPE] ... + +example scoped-action: error: Unrecognized arguments: bar. +" +`; exports[`CommandLineParser throws on unknown scoped arg 1`] = ` "example scoped-action --scope foo --: error: Unrecognized arguments: --scoped-bar baz. From 5d79789081603e615f5c43d2deb45b45a23246fb Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 23 Oct 2023 13:02:06 -0700 Subject: [PATCH 02/18] Rush change --- ...PreventAmbiguousAbbreviations_2023-10-23-20-01.json | 10 ++++++++++ ...PreventAmbiguousAbbreviations_2023-10-23-20-01.json | 10 ++++++++++ 2 files changed, 20 insertions(+) create mode 100644 common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json create mode 100644 common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json diff --git a/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json new file mode 100644 index 00000000000..361dc5c7c41 --- /dev/null +++ b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/heft", + "comment": "Fix an issue with parsing of the \"--debug\" and \"--unmanaged\" flags for Heft", + "type": "patch" + } + ], + "packageName": "@rushstack/heft" +} \ No newline at end of file diff --git a/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json new file mode 100644 index 00000000000..e7157127c1b --- /dev/null +++ b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/ts-command-line", + "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations", + "type": "minor" + } + ], + "packageName": "@rushstack/ts-command-line" +} \ No newline at end of file From 14cb91df60df452930c5b22219df482c3f0ba429 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 23 Oct 2023 13:32:10 -0700 Subject: [PATCH 03/18] Fix API --- common/reviews/api/ts-command-line.api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index 6a684ec766f..b9e3d8f833a 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -14,7 +14,7 @@ export class AliasCommandLineAction extends CommandLineAction { // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; // @internal (undocumented) - _registerDefinedParameters(existingParameterNames: Set): void; + _registerDefinedParameters(existingParameterNames?: Set): void; readonly targetAction: CommandLineAction; } From 08564ba40ca26f44f386f6183e7a01ca46b05ff9 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 23 Oct 2023 14:26:00 -0700 Subject: [PATCH 04/18] Change name of field --- .../src/providers/AliasCommandLineAction.ts | 2 +- .../src/providers/CommandLineParameterProvider.ts | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts index 8832809d15d..c8bc591c8b0 100644 --- a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts @@ -148,7 +148,7 @@ export class AliasCommandLineAction extends CommandLineAction { } // Also re-use the target action ambiguous parameters for this action - for (const [ambiguousParameterName, parserKey] of this.targetAction._ambiguousParameterParserKeys) { + for (const [ambiguousParameterName, parserKey] of this.targetAction._ambiguousParameterParserKeysByName) { const aliasParserKey: string = this._defineAmbiguousParameter(ambiguousParameterName); this._parameterKeyMap.set(aliasParserKey, parserKey); } diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 3713be958d5..0dd3a61a27f 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -75,7 +75,7 @@ export abstract class CommandLineParameterProvider { private static _keyCounter: number = 0; /** @internal */ - public readonly _ambiguousParameterParserKeys: Map; + public readonly _ambiguousParameterParserKeysByName: Map; /** @internal */ protected readonly _registeredParameterNames: Set = new Set(); /** @internal */ @@ -98,7 +98,7 @@ export abstract class CommandLineParameterProvider { this._parametersByLongName = new Map(); this._parametersByShortName = new Map(); this._parameterGroupsByName = new Map(); - this._ambiguousParameterParserKeys = new Map(); + this._ambiguousParameterParserKeysByName = new Map(); this._parametersRegistered = false; this._parametersProcessed = false; } @@ -457,7 +457,7 @@ export abstract class CommandLineParameterProvider { // We also need to loop through the defined ambiguous parameters and register them. These will be reported // as errors if the user attempts to use them. - for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeys) { + for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeysByName) { this._registerAmbiguousParameter(ambiguousParameterName, parserKey); } @@ -498,7 +498,7 @@ export abstract class CommandLineParameterProvider { } // Search for any ambiguous parameters and throw an error if any are found - for (const [parameterName, parserKey] of this._ambiguousParameterParserKeys) { + for (const [parameterName, parserKey] of this._ambiguousParameterParserKeysByName) { if (data[parserKey]) { // Write out the usage text to make it easier for the user to find the correct parameter name const errorPrefix: string = `${this.renderUsageText()}\n${parserOptions.toolFilename} ${ @@ -638,10 +638,10 @@ export abstract class CommandLineParameterProvider { // Generate and set the parser key at definition time. Only generate a new parser key // if the ambiguous paramter hasn't been defined yet. - let existingParserKey: string | undefined = this._ambiguousParameterParserKeys.get(name); + let existingParserKey: string | undefined = this._ambiguousParameterParserKeysByName.get(name); if (!existingParserKey) { existingParserKey = this._generateKey(); - this._ambiguousParameterParserKeys.set(name, existingParserKey); + this._ambiguousParameterParserKeysByName.set(name, existingParserKey); } return existingParserKey; } From 905f944d0ed55cb876be4d4e48fe1a4c75759654 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 23 Oct 2023 16:23:35 -0700 Subject: [PATCH 05/18] Update to handle ambiguous parameters at runtime, not define time --- common/reviews/api/ts-command-line.api.md | 8 +-- .../src/parameters/BaseClasses.ts | 8 --- .../src/providers/AliasCommandLineAction.ts | 29 +++++---- .../providers/CommandLineParameterProvider.ts | 59 +++++++++++++------ .../src/providers/CommandLineParser.ts | 2 +- .../src/providers/ScopedCommandLineAction.ts | 2 +- .../test/AmbiguousCommandLineParser.test.ts | 16 ++--- 7 files changed, 72 insertions(+), 52 deletions(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index b9e3d8f833a..eff79057e58 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -121,8 +121,6 @@ export abstract class CommandLineParameter { constructor(definition: IBaseCommandLineDefinition); abstract appendToArgList(argList: string[]): void; readonly description: string; - // @internal - _disableShortName(): void; readonly environmentVariable: string | undefined; // @internal _getSupplementaryNotes(supplementaryNotes: string[]): void; @@ -159,7 +157,7 @@ export abstract class CommandLineParameterProvider { // @internal constructor(); // @internal (undocumented) - readonly _ambiguousParameterParserKeys: Map; + readonly _ambiguousParameterParserKeysByName: Map; // @internal (undocumented) protected _defineAmbiguousParameter(name: string): string; defineChoiceListParameter(definition: ICommandLineChoiceListDefinition): CommandLineChoiceListParameter; @@ -195,9 +193,9 @@ export abstract class CommandLineParameterProvider { // @internal (undocumented) _registerDefinedParameters(existingParameterNames?: Set): void; // @internal (undocumented) - protected readonly _registeredParameterNames: Set; + protected readonly _registeredParameterParserKeysByName: Map; // @internal (undocumented) - protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean): void; + protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean, ignoreShortName: boolean): void; get remainder(): CommandLineRemainder | undefined; renderHelpText(): string; renderUsageText(): string; diff --git a/libraries/ts-command-line/src/parameters/BaseClasses.ts b/libraries/ts-command-line/src/parameters/BaseClasses.ts index 555c0e2196d..829039eba4c 100644 --- a/libraries/ts-command-line/src/parameters/BaseClasses.ts +++ b/libraries/ts-command-line/src/parameters/BaseClasses.ts @@ -179,14 +179,6 @@ export abstract class CommandLineParameter { */ public abstract _setValue(data: any): void; // eslint-disable-line @typescript-eslint/no-explicit-any - /** - * Called internally by CommandLineParameterProvider._registerDefinedParameters() - * @internal - */ - public _disableShortName(): void { - this._shortNameValue = undefined; - } - /** * Returns additional text used by the help formatter. * @internal diff --git a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts index c8bc591c8b0..06e90b6fd7b 100644 --- a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts @@ -87,11 +87,6 @@ export class AliasCommandLineAction extends CommandLineAction { /** @internal */ public _registerDefinedParameters(existingParameterNames?: Set): void { /* override */ - // First, we need to make sure that the target action is registered, since we need to re-use - // its parameters, and some of these only get defined during registration. This will no-op if - // the target action is already registered. - this.targetAction._registerDefinedParameters(existingParameterNames); - // All parameters are going to be defined by the target action. Re-use the target action parameters // for this action. for (const parameter of this.targetAction.parameters) { @@ -142,17 +137,12 @@ export class AliasCommandLineAction extends CommandLineAction { default: throw new Error(`Unsupported parameter kind: ${parameter.kind}`); } + // We know the parserKey is defined because the underlying _defineParameter method sets it, // and all parameters that we have access to have already been defined. this._parameterKeyMap.set(aliasParameter._parserKey!, parameter._parserKey!); } - // Also re-use the target action ambiguous parameters for this action - for (const [ambiguousParameterName, parserKey] of this.targetAction._ambiguousParameterParserKeysByName) { - const aliasParserKey: string = this._defineAmbiguousParameter(ambiguousParameterName); - this._parameterKeyMap.set(aliasParserKey, parserKey); - } - // We also need to register the remainder parameter if the target action has one. The parser // key for this parameter is constant. if (this.targetAction.remainder) { @@ -160,8 +150,23 @@ export class AliasCommandLineAction extends CommandLineAction { this._parameterKeyMap.set(argparse.Const.REMAINDER, argparse.Const.REMAINDER); } - // Finally, register the parameters with the parser. + // Finally, register the parameters with the parser. We need to make sure that the target action + // is registered, since we need to re-use its parameters, and ambiguous parameters are discovered + // during registration. This will no-op if the target action is already registered. + this.targetAction._registerDefinedParameters(existingParameterNames); super._registerDefinedParameters(existingParameterNames); + + // We need to re-map the ambiguous parameters after they are defined by calling + // super._registerDefinedParameters() + for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeysByName) { + const targetParserKey: string | undefined = + this.targetAction._ambiguousParameterParserKeysByName.get(ambiguousParameterName); + + // If we have a mapping for the specified key, then use it. Otherwise, use the key as-is. + if (targetParserKey) { + this._parameterKeyMap.set(parserKey, targetParserKey); + } + } } /** diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 0dd3a61a27f..7e54826c667 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -77,7 +77,7 @@ export abstract class CommandLineParameterProvider { /** @internal */ public readonly _ambiguousParameterParserKeysByName: Map; /** @internal */ - protected readonly _registeredParameterNames: Set = new Set(); + protected readonly _registeredParameterParserKeysByName: Map; /** @internal */ protected _parametersRegistered: boolean; @@ -99,6 +99,7 @@ export abstract class CommandLineParameterProvider { this._parametersByShortName = new Map(); this._parameterGroupsByName = new Map(); this._ambiguousParameterParserKeysByName = new Map(); + this._registeredParameterParserKeysByName = new Map(); this._parametersRegistered = false; this._parametersProcessed = false; } @@ -418,20 +419,15 @@ export abstract class CommandLineParameterProvider { return; } - // Register the existing parameters as ambiguous parameters first. These are generally provided by the - // parent action. Register these first so that we can report matching parameters as errors. - for (const existingParameterName of existingParameterNames || []) { - this._defineAmbiguousParameter(existingParameterName); - } - // First, loop through all parameters with short names. If there are any duplicates, disable the short names // since we can't prefix scopes to short names in order to deduplicate them. The duplicate short names will // be reported as errors if the user attempts to use them. + const parametersWithDuplicateShortNames: Set = new Set(); for (const [shortName, shortNameParameters] of this._parametersByShortName.entries()) { if (shortNameParameters.length > 1) { for (const parameter of shortNameParameters) { this._defineAmbiguousParameter(shortName); - parameter._disableShortName(); + parametersWithDuplicateShortNames.add(parameter); } } } @@ -451,14 +447,27 @@ export abstract class CommandLineParameterProvider { } this._defineAmbiguousParameter(parameter.longName); } - this._registerParameter(parameter, useScopedLongName); + + const ignoreShortName: boolean = parametersWithDuplicateShortNames.has(parameter); + this._registerParameter(parameter, useScopedLongName, ignoreShortName); } } + // Register the existing parameters as ambiguous parameters. These are generally provided by the + // parent action. + for (const existingParameterName of existingParameterNames || []) { + this._defineAmbiguousParameter(existingParameterName); + } + // We also need to loop through the defined ambiguous parameters and register them. These will be reported // as errors if the user attempts to use them. for (const [ambiguousParameterName, parserKey] of this._ambiguousParameterParserKeysByName) { - this._registerAmbiguousParameter(ambiguousParameterName, parserKey); + // Only register the ambiguous parameter if it hasn't already been registered. We will still handle these + // already-registered parameters as ambiguous, but by avoiding registering again, we will defer errors + // until the user actually attempts to use the parameter. + if (!this._registeredParameterParserKeysByName.has(ambiguousParameterName)) { + this._registerAmbiguousParameter(ambiguousParameterName, parserKey); + } } // Need to add the remainder parameter last @@ -506,6 +515,15 @@ export abstract class CommandLineParameterProvider { }: error: `; const errorPostfix: string = '\n'; + // When the parser key matches the actually registered parameter, we know that this is an ambiguous + // parameter sourced from the parent action or tool + if (this._registeredParameterParserKeysByName.get(parameterName) === parserKey) { + throw new CommandLineParserExitError( + 1, + `${errorPrefix}The parameter name "${parameterName}" is ambiguous.${errorPostfix}` + ); + } + // Determine if the ambiguous parameter is a short name or a long name, since the process of finding // the non-ambiguous name is different for each. const duplicateShortNameParameters: CommandLineParameter[] | undefined = @@ -576,6 +594,7 @@ export abstract class CommandLineParameterProvider { ); } + // This shouldn't happen, but we also shouldn't allow the user to use the ambiguous parameter throw new CommandLineParserExitError( 1, `${errorPrefix}The parameter name "${parameterName}" is ambiguous.${errorPostfix}` @@ -636,20 +655,26 @@ export abstract class CommandLineParameterProvider { throw new Error('Parameters have already been registered for this provider'); } - // Generate and set the parser key at definition time. Only generate a new parser key - // if the ambiguous paramter hasn't been defined yet. - let existingParserKey: string | undefined = this._ambiguousParameterParserKeysByName.get(name); + // Only generate a new parser key if the ambiguous parameter hasn't been defined yet, + // either as an existing parameter or as another ambiguous parameter + let existingParserKey: string | undefined = this._registeredParameterParserKeysByName.get(name); + this._ambiguousParameterParserKeysByName.get(name); if (!existingParserKey) { existingParserKey = this._generateKey(); - this._ambiguousParameterParserKeysByName.set(name, existingParserKey); } + + this._ambiguousParameterParserKeysByName.set(name, existingParserKey); return existingParserKey; } /** @internal */ - protected _registerParameter(parameter: CommandLineParameter, useScopedLongName: boolean): void { + protected _registerParameter( + parameter: CommandLineParameter, + useScopedLongName: boolean, + ignoreShortName: boolean + ): void { const names: string[] = []; - if (parameter.shortName) { + if (parameter.shortName && !ignoreShortName) { names.push(parameter.shortName); } @@ -747,7 +772,7 @@ export abstract class CommandLineParameterProvider { // Register the parameter names so that we can detect ambiguous parameters for (const name of [...names, ...(parameter.undocumentedSynonyms || [])]) { - this._registeredParameterNames.add(name); + this._registeredParameterParserKeysByName.set(name, parameter._parserKey!); } } diff --git a/libraries/ts-command-line/src/providers/CommandLineParser.ts b/libraries/ts-command-line/src/providers/CommandLineParser.ts index 20556981c2f..7a34f05ca3b 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParser.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParser.ts @@ -265,7 +265,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { public _registerDefinedParameters(): void { super._registerDefinedParameters(); for (const action of this._actions) { - action._registerDefinedParameters(this._registeredParameterNames); + action._registerDefinedParameters(new Set(this._registeredParameterParserKeysByName.keys())); } } diff --git a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts index b3715f89eb1..59cb97a5d27 100644 --- a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts @@ -200,7 +200,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { public _registerDefinedParameters(existingParameterNames?: Set): void { super._registerDefinedParameters(existingParameterNames); - for (const registeredParameterName of this._registeredParameterNames) { + for (const registeredParameterName of this._registeredParameterParserKeysByName.keys()) { this._existingParameterNames.add(registeredParameterName); } diff --git a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts index df2950be607..84b23bf26c0 100644 --- a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts +++ b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts @@ -296,16 +296,16 @@ describe(`Ambiguous ${CommandLineParser.name}`, () => { ).rejects.toThrowErrorMatchingSnapshot(); }); - it('fails when an action declares a flag that was declared in the tool', async () => { + it('fails when providing a flag to an action that was also declared in the tool', async () => { const commandLineParser: GenericCommandLine = new GenericCommandLine(AbbreviationAction); commandLineParser.defineFlagParameter({ parameterLongName: '--abbreviation-flag', description: 'A flag used to test abbreviation logic' }); - expect(commandLineParser._registerDefinedParameters.bind(commandLineParser)).toThrowError( - /Conflicting option string\(s\): --abbreviation-flag/ - ); + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbreviation-flag']) + ).rejects.toThrowError(/The parameter name "--abbreviation-flag" is ambiguous/); }); it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { @@ -404,7 +404,7 @@ describe(`Ambiguous aliased ${CommandLineParser.name}`, () => { ).rejects.toThrowErrorMatchingSnapshot(); }); - it('fails when an action declares a flag that was declared in the tool', async () => { + it('fails when providing a flag to an action that was also declared in the tool', async () => { const commandLineParser: GenericCommandLine = new GenericCommandLine(AliasAction, AbbreviationAction); commandLineParser.addAction( (commandLineParser.getAction('do:the-job-alias')! as AliasAction).targetAction @@ -414,9 +414,9 @@ describe(`Ambiguous aliased ${CommandLineParser.name}`, () => { description: 'A flag used to test abbreviation logic' }); - expect(commandLineParser._registerDefinedParameters.bind(commandLineParser)).toThrowError( - /Conflicting option string\(s\): --abbreviation-flag/ - ); + await expect( + commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbreviation-flag']) + ).rejects.toThrowError(/The parameter name "--abbreviation-flag" is ambiguous/); }); it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { From 4f4eca5311cbd54359813f7e28fd9227ba2c902e Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Tue, 24 Oct 2023 09:49:47 -0700 Subject: [PATCH 06/18] Defer handling of ambiguous options to runtime instead of define-time --- common/reviews/api/ts-command-line.api.md | 2 +- .../src/providers/CommandLineParameterProvider.ts | 12 +++++++----- .../src/providers/CommandLineParser.ts | 13 +++++++++---- .../src/providers/ScopedCommandLineAction.ts | 9 +-------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index eff79057e58..b35dae028e1 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -221,7 +221,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { protected _getArgumentParser(): argparse.ArgumentParser; protected onExecute(): Promise; // @internal (undocumented) - _registerDefinedParameters(): void; + _registerDefinedParameters(existingParameterNames?: Set): void; selectedAction: CommandLineAction | undefined; tryGetAction(actionName: string): CommandLineAction | undefined; } diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 7e54826c667..48acdd3e464 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -510,9 +510,10 @@ export abstract class CommandLineParameterProvider { for (const [parameterName, parserKey] of this._ambiguousParameterParserKeysByName) { if (data[parserKey]) { // Write out the usage text to make it easier for the user to find the correct parameter name - const errorPrefix: string = `${this.renderUsageText()}\n${parserOptions.toolFilename} ${ - data.aliasAction || data.action - }: error: `; + const errorPrefix: string = + `${this.renderUsageText()}\n${parserOptions.toolFilename}` + + // Handle aliases, actions, and actionless parameter providers + `${data.aliasAction || data.action ? ' ' : ''}${data.aliasAction || data.action || ''}: error: `; const errorPostfix: string = '\n'; // When the parser key matches the actually registered parameter, we know that this is an ambiguous @@ -657,8 +658,9 @@ export abstract class CommandLineParameterProvider { // Only generate a new parser key if the ambiguous parameter hasn't been defined yet, // either as an existing parameter or as another ambiguous parameter - let existingParserKey: string | undefined = this._registeredParameterParserKeysByName.get(name); - this._ambiguousParameterParserKeysByName.get(name); + let existingParserKey: string | undefined = + this._registeredParameterParserKeysByName.get(name) || + this._ambiguousParameterParserKeysByName.get(name); if (!existingParserKey) { existingParserKey = this._generateKey(); } diff --git a/libraries/ts-command-line/src/providers/CommandLineParser.ts b/libraries/ts-command-line/src/providers/CommandLineParser.ts index 7a34f05ca3b..b6af2e7fa1b 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParser.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParser.ts @@ -243,7 +243,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { } this.selectedAction?._processParsedData(this._options, data); - return this.onExecute(); + await this.onExecute(); } catch (err) { if (err instanceof CommandLineParserExitError) { if (!err.exitCode) { @@ -262,10 +262,15 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { } /** @internal */ - public _registerDefinedParameters(): void { - super._registerDefinedParameters(); + public _registerDefinedParameters(existingParameterNames?: Set): void { + super._registerDefinedParameters(existingParameterNames); + + const registeredParameterNames: Set = new Set([ + ...(existingParameterNames || []), + ...this._registeredParameterParserKeysByName.keys() + ]); for (const action of this._actions) { - action._registerDefinedParameters(new Set(this._registeredParameterParserKeysByName.keys())); + action._registerDefinedParameters(registeredParameterNames); } } diff --git a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts index 59cb97a5d27..8786be4bd7e 100644 --- a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts @@ -60,14 +60,7 @@ class InternalScopedCommandLineParser extends CommandLineParser { } public _registerDefinedParameters(): void { - if (!this._parametersRegistered) { - // Manually register our ambiguous parameters from the parent tool and action - for (const existingParameterName of this._internalOptions.existingParameterNames || []) { - this._defineAmbiguousParameter(existingParameterName); - } - } - - super._registerDefinedParameters(); + super._registerDefinedParameters(this._internalOptions.existingParameterNames); } protected async onExecute(): Promise { From 6fb3ef4969a46f28eb85db0b473004764acf6e94 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Tue, 24 Oct 2023 09:50:15 -0700 Subject: [PATCH 07/18] Unify parsing of initial Heft tool parameters and remove argparse --- apps/heft/package.json | 2 -- apps/heft/src/cli/HeftCommandLineParser.ts | 16 +++++++-------- apps/heft/src/startWithVersionSelector.ts | 19 ++++------------- apps/heft/src/utilities/CliUtilities.ts | 22 ++++++++++++++++++++ common/config/rush/pnpm-lock.yaml | 24 +++++++++++----------- common/config/rush/repo-state.json | 2 +- 6 files changed, 46 insertions(+), 39 deletions(-) create mode 100644 apps/heft/src/utilities/CliUtilities.ts diff --git a/apps/heft/package.json b/apps/heft/package.json index 3614975c776..610d7009141 100644 --- a/apps/heft/package.json +++ b/apps/heft/package.json @@ -40,7 +40,6 @@ "@rushstack/rig-package": "workspace:*", "@rushstack/ts-command-line": "workspace:*", "@types/tapable": "1.0.6", - "argparse": "~1.0.9", "chokidar": "~3.4.0", "fast-glob": "~3.3.1", "git-repo-info": "~2.1.0", @@ -54,7 +53,6 @@ "local-eslint-config": "workspace:*", "@rushstack/heft": "0.62.0", "@rushstack/heft-node-rig": "2.3.2", - "@types/argparse": "1.0.38", "@types/heft-jest": "1.0.1", "@types/node": "18.17.15", "@types/watchpack": "2.4.0", diff --git a/apps/heft/src/cli/HeftCommandLineParser.ts b/apps/heft/src/cli/HeftCommandLineParser.ts index fab3b3c10b9..e26fd94d9b0 100644 --- a/apps/heft/src/cli/HeftCommandLineParser.ts +++ b/apps/heft/src/cli/HeftCommandLineParser.ts @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. -import { ArgumentParser } from 'argparse'; import { CommandLineParser, type AliasCommandLineAction, @@ -20,12 +19,13 @@ import { MetricsCollector } from '../metrics/MetricsCollector'; import { HeftConfiguration } from '../configuration/HeftConfiguration'; import { InternalHeftSession } from '../pluginFramework/InternalHeftSession'; import { LoggingManager } from '../pluginFramework/logging/LoggingManager'; -import { Constants } from '../utilities/Constants'; import { CleanAction } from './actions/CleanAction'; import { PhaseAction } from './actions/PhaseAction'; import { RunAction } from './actions/RunAction'; import type { IHeftActionOptions } from './actions/IHeftAction'; import { AliasAction } from './actions/AliasAction'; +import { getToolParametersFromArgs } from '../utilities/CliUtilities'; +import { Constants } from '../utilities/Constants'; /** * This interfaces specifies values for parameters that must be parsed before the CLI @@ -233,13 +233,11 @@ export class HeftCommandLineParser extends CommandLineParser { throw new InternalError('onDefineParameters() has not yet been called.'); } - // This is a rough parsing of the --debug parameter - const parser: ArgumentParser = new ArgumentParser({ addHelp: false }); - parser.addArgument(this._debugFlag.longName, { dest: 'debug', action: 'storeTrue' }); - parser.addArgument(this._unmanagedFlag.longName, { dest: 'unmanaged', action: 'storeTrue' }); - - const [result]: IPreInitializationArgumentValues[] = parser.parseKnownArgs(args); - return result; + const toolParameters: Set = getToolParametersFromArgs(args); + return { + debug: toolParameters.has(this._debugFlag.longName), + unmanaged: toolParameters.has(this._unmanagedFlag.longName) + }; } private async _reportErrorAndSetExitCode(error: Error): Promise { diff --git a/apps/heft/src/startWithVersionSelector.ts b/apps/heft/src/startWithVersionSelector.ts index 160d7671c16..561793ec41d 100644 --- a/apps/heft/src/startWithVersionSelector.ts +++ b/apps/heft/src/startWithVersionSelector.ts @@ -8,6 +8,7 @@ import * as path from 'path'; import * as fs from 'fs'; import type { IPackageJson } from '@rushstack/node-core-library'; +import { getToolParametersFromArgs } from './utilities/CliUtilities'; import { Constants } from './utilities/Constants'; const HEFT_PACKAGE_NAME: string = '@rushstack/heft'; @@ -49,27 +50,15 @@ function tryGetPackageFolderFor(resolvedFileOrFolderPath: string): string | unde * Use "heft --unmanaged" to bypass this feature. */ function tryStartLocalHeft(): boolean { - const toolArgs: Set = new Set(); - // Skip the first two arguments, which are the path to the Node executable and the path to the Heft - // entrypoint. The remaining arguments are the tool arguments. Grab them until we reach a non-"--"-prefixed - // argument. We can do this simple parsing because the Heft tool only has simple optional flags. - for (let i: number = 2; i < process.argv.length; ++i) { - const arg: string = process.argv[i]; - if (!arg.startsWith('--')) { - break; - } - - toolArgs.add(arg); - } - - if (toolArgs.has(Constants.unmanagedParameterLongName)) { + const toolParameters: Set = getToolParametersFromArgs(); + if (toolParameters.has(Constants.unmanagedParameterLongName)) { console.log( `Bypassing the Heft version selector because ${JSON.stringify(Constants.unmanagedParameterLongName)} ` + 'was specified.' ); console.log(); return false; - } else if (toolArgs.has(Constants.debugParameterLongName)) { + } else if (toolParameters.has(Constants.debugParameterLongName)) { // The unmanaged flag could be undiscoverable if it's not in their locally installed version console.log( 'Searching for a locally installed version of Heft. Use the ' + diff --git a/apps/heft/src/utilities/CliUtilities.ts b/apps/heft/src/utilities/CliUtilities.ts new file mode 100644 index 00000000000..30f1786667f --- /dev/null +++ b/apps/heft/src/utilities/CliUtilities.ts @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. +// See LICENSE in the project root for license information. + +/** + * Parse the arguments to the tool being executed and return the tool arguments. + * + * @param argv - The arguments to parse. Defaults to `process.argv`. + */ +export function getToolParametersFromArgs(argv: string[] = process.argv): Set { + const toolParameters: Set = new Set(); + // Skip the first two arguments, which are the path to the Node executable and the path to the Heft + // entrypoint. The remaining arguments are the tool arguments. Grab them until we reach a non-"-"-prefixed + // argument. We can do this simple parsing because the Heft tool only has simple optional flags. + for (let i: number = 2; i < argv.length; ++i) { + const arg: string = argv[i]; + if (!arg.startsWith('-')) { + break; + } + toolParameters.add(arg); + } + return toolParameters; +} diff --git a/common/config/rush/pnpm-lock.yaml b/common/config/rush/pnpm-lock.yaml index 971e5785f95..21870ea5d31 100644 --- a/common/config/rush/pnpm-lock.yaml +++ b/common/config/rush/pnpm-lock.yaml @@ -134,9 +134,6 @@ importers: '@types/tapable': specifier: 1.0.6 version: 1.0.6 - argparse: - specifier: ~1.0.9 - version: 1.0.10 chokidar: specifier: ~3.4.0 version: 3.4.3 @@ -168,9 +165,6 @@ importers: '@rushstack/heft-node-rig': specifier: 2.3.2 version: 2.3.2(@rushstack/heft@0.62.0)(@types/node@18.17.15) - '@types/argparse': - specifier: 1.0.38 - version: 1.0.38 '@types/heft-jest': specifier: 1.0.1 version: 1.0.1 @@ -1743,10 +1737,10 @@ importers: version: link:../../heft-plugins/heft-typescript-plugin '@types/jest': specifier: ts4.9 - version: 29.5.5 + version: 29.5.6 '@types/node': specifier: ts4.9 - version: 20.8.3 + version: 20.8.7 eslint: specifier: ~8.7.0 version: 8.7.0 @@ -11283,8 +11277,8 @@ packages: expect: 29.6.2 pretty-format: 29.6.2 - /@types/jest@29.5.5: - resolution: {integrity: sha512-ebylz2hnsWR9mYvmBFbXJXr+33UPc4+ZdxyDXh5w0FlPBTfCVN3wPL+kuOiQt3xvrK419v7XWeAs+AeOksafXg==} + /@types/jest@29.5.6: + resolution: {integrity: sha512-/t9NnzkOpXb4Nfvg17ieHE6EeSjDS2SGSpNYfoLbUAeL/EOueU/RSdOWFpfQTXBEM7BguYW1XQ0EbM+6RlIh6w==} dependencies: expect: 29.6.2 pretty-format: 29.6.2 @@ -11396,8 +11390,10 @@ packages: /@types/node@18.17.15: resolution: {integrity: sha512-2yrWpBk32tvV/JAd3HNHWuZn/VDN1P+72hWirHnvsvTGSqbANi+kSeuQR9yAHnbvaBvHDsoTdXV0Fe+iRtHLKA==} - /@types/node@20.8.3: - resolution: {integrity: sha512-jxiZQFpb+NlH5kjW49vXxvxTjeeqlbsnTAdBTKpzEdPs9itay7MscYXz3Fo9VYFEsfQ6LJFitHad3faerLAjCw==} + /@types/node@20.8.7: + resolution: {integrity: sha512-21TKHHh3eUHIi2MloeptJWALuCu5H7HQTdTrWIFReA8ad+aggoX+lRes3ex7/FtpC+sVUpFMQ+QTfYr74mruiQ==} + dependencies: + undici-types: 5.25.3 dev: true /@types/normalize-package-data@2.4.1: @@ -24874,6 +24870,10 @@ packages: resolution: {integrity: sha512-+A5Sja4HP1M08MaXya7p5LvjuM7K6q/2EaC0+iovj/wOcMsTzMvDFbasi/oSapiwOlt252IqsKqPjCl7huKS0A==} dev: true + /undici-types@5.25.3: + resolution: {integrity: sha512-Ga1jfYwRn7+cP9v8auvEXN1rX3sWqlayd4HP7OKk4mZWylEmu3KzXDUGrQUN6Ol7qo1gPvB2e5gX6udnyEPgdA==} + dev: true + /unfetch@4.2.0: resolution: {integrity: sha512-F9p7yYCn6cIW9El1zi0HI6vqpeIvBsr3dSuRO6Xuppb1u5rXpCPmMvLSyECLhybr9isec8Ohl0hPekMVrEinDA==} dev: true diff --git a/common/config/rush/repo-state.json b/common/config/rush/repo-state.json index 973b6d06588..2ec955a05d6 100644 --- a/common/config/rush/repo-state.json +++ b/common/config/rush/repo-state.json @@ -1,5 +1,5 @@ // DO NOT MODIFY THIS FILE MANUALLY BUT DO COMMIT IT. It is generated and used by Rush. { - "pnpmShrinkwrapHash": "1df1019398400392ff48b9031f544c287bfe1e7b", + "pnpmShrinkwrapHash": "8705cc33c69cd8cec19072988855c937821ff378", "preferredVersionsHash": "1926a5b12ac8f4ab41e76503a0d1d0dccc9c0e06" } From 08efd1420c2ddbb5249560d44a7db93ef3ee63b0 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Tue, 24 Oct 2023 10:16:18 -0700 Subject: [PATCH 08/18] Cleanup --- .../providers/CommandLineParameterProvider.ts | 55 ++++++++++--------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 48acdd3e464..880bae1aded 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -509,20 +509,10 @@ export abstract class CommandLineParameterProvider { // Search for any ambiguous parameters and throw an error if any are found for (const [parameterName, parserKey] of this._ambiguousParameterParserKeysByName) { if (data[parserKey]) { - // Write out the usage text to make it easier for the user to find the correct parameter name - const errorPrefix: string = - `${this.renderUsageText()}\n${parserOptions.toolFilename}` + - // Handle aliases, actions, and actionless parameter providers - `${data.aliasAction || data.action ? ' ' : ''}${data.aliasAction || data.action || ''}: error: `; - const errorPostfix: string = '\n'; - // When the parser key matches the actually registered parameter, we know that this is an ambiguous // parameter sourced from the parent action or tool if (this._registeredParameterParserKeysByName.get(parameterName) === parserKey) { - throw new CommandLineParserExitError( - 1, - `${errorPrefix}The parameter name "${parameterName}" is ambiguous.${errorPostfix}` - ); + this._throwParserExitError(parserOptions, data, 1, `Ambiguous option: "${parameterName}".`); } // Determine if the ambiguous parameter is a short name or a long name, since the process of finding @@ -559,12 +549,12 @@ export abstract class CommandLineParameterProvider { // Throw an error including the non-ambiguous long names for the parameters that have the ambiguous // short name, ex. - // Error: The short parameter name "-p" is ambiguous. It could refer to any of the following - // parameters: "--param1", "--param2" - throw new CommandLineParserExitError( + // Error: Ambiguous option "-p" could match "--param1", "--param2" + this._throwParserExitError( + parserOptions, + data, 1, - `${errorPrefix}The short parameter name "${parameterName}" is ambiguous. It could refer to any of ` + - `the following parameters: "${nonAmbiguousLongNames.join('", "')}"${errorPostfix}` + `Ambiguous option: "${parameterName}" could match ${nonAmbiguousLongNames.join(', ')}.` ); } @@ -586,20 +576,17 @@ export abstract class CommandLineParameterProvider { // Throw an error including the non-ambiguous scoped long names for the parameters that have the // ambiguous long name, ex. - // Error: The parameter name "--param" is ambiguous. It could refer to any of the following - // parameters: "--scope1:param", "--scope2:param" - throw new CommandLineParserExitError( + // Error: Ambiguous option: "--param" could match --scope1:param, --scope2:param + this._throwParserExitError( + parserOptions, + data, 1, - `${errorPrefix}The parameter name "${parameterName}" is ambiguous. It could refer to any of ` + - `the following parameters: "${nonAmbiguousLongNames.join('", "')}"${errorPostfix}` + `Ambiguous option: "${parameterName}" could match ${nonAmbiguousLongNames.join(', ')}.` ); } // This shouldn't happen, but we also shouldn't allow the user to use the ambiguous parameter - throw new CommandLineParserExitError( - 1, - `${errorPrefix}The parameter name "${parameterName}" is ambiguous.${errorPostfix}` - ); + this._throwParserExitError(parserOptions, data, 1, `Ambiguous option: "${parameterName}".`); } } @@ -832,4 +819,22 @@ export abstract class CommandLineParameterProvider { return parameter as T; } + + private _throwParserExitError( + parserOptions: ICommandLineParserOptions, + data: ICommandLineParserData, + errorCode: number, + message: string + ): never { + // Write out the usage text to make it easier for the user to find the correct parameter name + const targetActionName: string = data.aliasAction || data.action || ''; + const errorPrefix: string = + `Error: ${parserOptions.toolFilename}` + + // Handle aliases, actions, and actionless parameter providers + `${targetActionName ? ' ' : ''}${targetActionName}: error: `; + + // eslint-disable-next-line no-console + console.log(this.renderUsageText()); + throw new CommandLineParserExitError(errorCode, `${errorPrefix}${message.trimStart().trimEnd()}\n`); + } } From 802f7d639fa2264e93a3ea26d0bfb631612f5cf3 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Tue, 24 Oct 2023 11:08:21 -0700 Subject: [PATCH 09/18] Fix tests --- .../test/AmbiguousCommandLineParser.test.ts | 12 ++--- .../AmbiguousCommandLineParser.test.ts.snap | 52 +++---------------- 2 files changed, 13 insertions(+), 51 deletions(-) diff --git a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts index 84b23bf26c0..fcb819333ac 100644 --- a/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts +++ b/libraries/ts-command-line/src/test/AmbiguousCommandLineParser.test.ts @@ -305,7 +305,7 @@ describe(`Ambiguous ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbreviation-flag']) - ).rejects.toThrowError(/The parameter name "--abbreviation-flag" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation-flag"/); }); it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { @@ -317,7 +317,7 @@ describe(`Ambiguous ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['do:the-job', '--abbreviation']) - ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation"/); }); it('fails when providing an ambiguous abbreviation between flags on the tool and the action', async () => { @@ -416,7 +416,7 @@ describe(`Ambiguous aliased ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbreviation-flag']) - ).rejects.toThrowError(/The parameter name "--abbreviation-flag" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation-flag"/); }); it('fails when providing an exact match to an ambiguous abbreviation between flags on the tool and the action', async () => { @@ -431,7 +431,7 @@ describe(`Ambiguous aliased ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['do:the-job-alias', '--abbreviation']) - ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation"/); }); it('fails when providing an ambiguous abbreviation between flags on the tool and the action', async () => { @@ -543,7 +543,7 @@ describe(`Ambiguous scoping ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbreviation']) - ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation"/); }); it('fails when providing an exact match to an ambiguous abbreviation between flags on the scoped action and the unscoped action', async () => { @@ -558,7 +558,7 @@ describe(`Ambiguous scoping ${CommandLineParser.name}`, () => { await expect( commandLineParser.executeWithoutErrorHandling(['scoped-action', '--scoping', '--', '--abbreviation']) - ).rejects.toThrowError(/The parameter name "--abbreviation" is ambiguous/); + ).rejects.toThrowError(/Ambiguous option: "--abbreviation"/); }); it('fails when providing an ambiguous abbreviation between flags on the tool and the scoped action', async () => { diff --git a/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap b/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap index 5ab8d467679..643a1e8175f 100644 --- a/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap +++ b/libraries/ts-command-line/src/test/__snapshots__/AmbiguousCommandLineParser.test.ts.snap @@ -30,22 +30,12 @@ Object { `; exports[`Ambiguous CommandLineParser fails to execute when an ambiguous long name is provided 1`] = ` -"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example do:the-job: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +"Error: example do:the-job: error: Ambiguous option: \\"--arg\\" could match --scope1:arg, --scope2:arg. " `; exports[`Ambiguous CommandLineParser fails to execute when an ambiguous short name is provided 1`] = ` -"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example do:the-job: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +"Error: example do:the-job: error: Ambiguous option: \\"-s\\" could match --short1, --short2. " `; @@ -79,54 +69,26 @@ Object { `; exports[`Ambiguous aliased CommandLineParser fails to execute when an ambiguous long name is provided 1`] = ` -"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example do:the-job-alias: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +"Error: example do:the-job-alias: error: Ambiguous option: \\"--arg\\" could match --scope1:arg, --scope2:arg. " `; exports[`Ambiguous aliased CommandLineParser fails to execute when an ambiguous short name is provided 1`] = ` -"usage: example do:the-job [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example do:the-job-alias: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +"Error: example do:the-job-alias: error: Ambiguous option: \\"-s\\" could match --short1, --short2. " `; exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous long name is provided to a scoping action 1`] = ` -"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] - [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example scoped-action --scoping -- undefined: error: The parameter name \\"--arg\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\" +"Error: example scoped-action --scoping --: error: Ambiguous option: \\"--arg\\" could match --scope1:arg, --scope2:arg. " `; exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action 1`] = ` -"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] - [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example scoped-action --scoping -- undefined: error: The short parameter name \\"-s\\" is ambiguous. It could refer to any of the following parameters: \\"--short1\\", \\"--short2\\" +"Error: example scoped-action --scoping --: error: Ambiguous option: \\"-s\\" could match --short1, --short2. " `; exports[`Ambiguous scoping CommandLineParser fails to execute when an ambiguous short name is provided to a scoping action with a matching ambiguous long name 1`] = ` -"usage: example scoped-action --scoping -- [-h] [--short1 ARG] [--short2 ARG] - [--scope1:arg ARG] - [--scope2:arg ARG] - [--non-conflicting-arg ARG] - - -example scoped-action --scoping -- undefined: error: The short parameter name \\"-a\\" is ambiguous. It could refer to any of the following parameters: \\"--scope1:arg\\", \\"--scope2:arg\\", \\"--non-conflicting-arg\\" +"Error: example scoped-action --scoping --: error: Ambiguous option: \\"-a\\" could match --scope1:arg, --scope2:arg, --non-conflicting-arg. " `; From a164382d874fadec5bd7b0b0dbe99816fd9f572c Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Tue, 24 Oct 2023 11:11:38 -0700 Subject: [PATCH 10/18] Remove "heft run" short-parameters for "--to", "--to-except", and "--only" --- apps/heft/src/cli/actions/RunAction.ts | 3 --- apps/heft/src/utilities/Constants.ts | 6 ------ ...PreventAmbiguousAbbreviations_2023-10-24-18-11.json | 10 ++++++++++ 3 files changed, 10 insertions(+), 9 deletions(-) create mode 100644 common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json diff --git a/apps/heft/src/cli/actions/RunAction.ts b/apps/heft/src/cli/actions/RunAction.ts index 638f76b3890..abdba41e2a2 100644 --- a/apps/heft/src/cli/actions/RunAction.ts +++ b/apps/heft/src/cli/actions/RunAction.ts @@ -78,21 +78,18 @@ export function definePhaseScopingParameters(action: IHeftAction): IScopingParam return { toParameter: action.defineStringListParameter({ parameterLongName: Constants.toParameterLongName, - parameterShortName: Constants.toParameterShortName, description: `The phase to ${action.actionName} to, including all transitive dependencies.`, argumentName: 'PHASE', parameterGroup: ScopedCommandLineAction.ScopingParameterGroup }), toExceptParameter: action.defineStringListParameter({ parameterLongName: Constants.toExceptParameterLongName, - parameterShortName: Constants.toExceptParameterShortName, description: `The phase to ${action.actionName} to (but not include), including all transitive dependencies.`, argumentName: 'PHASE', parameterGroup: ScopedCommandLineAction.ScopingParameterGroup }), onlyParameter: action.defineStringListParameter({ parameterLongName: Constants.onlyParameterLongName, - parameterShortName: Constants.onlyParameterShortName, description: `The phase to ${action.actionName}.`, argumentName: 'PHASE', parameterGroup: ScopedCommandLineAction.ScopingParameterGroup diff --git a/apps/heft/src/utilities/Constants.ts b/apps/heft/src/utilities/Constants.ts index 5944f2fb451..b7b794ae91a 100644 --- a/apps/heft/src/utilities/Constants.ts +++ b/apps/heft/src/utilities/Constants.ts @@ -20,18 +20,12 @@ export class Constants { public static onlyParameterLongName: string = '--only'; - public static onlyParameterShortName: string = '-o'; - public static productionParameterLongName: string = '--production'; public static toParameterLongName: string = '--to'; - public static toParameterShortName: string = '-t'; - public static toExceptParameterLongName: string = '--to-except'; - public static toExceptParameterShortName: string = '-T'; - public static unmanagedParameterLongName: string = '--unmanaged'; public static verboseParameterLongName: string = '--verbose'; diff --git a/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json new file mode 100644 index 00000000000..4622de5a60b --- /dev/null +++ b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@rushstack/heft", + "comment": "[BREAKING CHANGE] Remove \"heft run\" short-parameters for \"--to\", \"--to-except\", and \"--only\"", + "type": "minor" + } + ], + "packageName": "@rushstack/heft" +} \ No newline at end of file From 1610a46b41e9e0c4d17be9618f9b7c8f4f6e2af7 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Wed, 25 Oct 2023 13:59:24 -0700 Subject: [PATCH 11/18] Clarify changelog --- ...r-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json index e7157127c1b..8e1587ae251 100644 --- a/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json +++ b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@rushstack/ts-command-line", - "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations", + "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations. For example, if a CLI tool `mytool` has a parameter `--myparam` and an action `myaction`, then `myaction` would not accept a parameter named `--myparam`. Additionally, any parameter that can be abbreviated to `--myparam` must be uniquely provided, ex. `--myparam-2` can only be abbreviated to `--myparam-`, since any shorter abbreviation would be ambiguous with the original `--myparam` on the tool.", "type": "minor" } ], From 8ebb25f973e2113b2aca919dc3b80c985bc5ceaa Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Wed, 25 Oct 2023 14:00:17 -0700 Subject: [PATCH 12/18] Rename function --- apps/heft/src/cli/HeftCommandLineParser.ts | 4 ++-- apps/heft/src/startWithVersionSelector.ts | 4 ++-- apps/heft/src/utilities/CliUtilities.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/heft/src/cli/HeftCommandLineParser.ts b/apps/heft/src/cli/HeftCommandLineParser.ts index e26fd94d9b0..fa34482423b 100644 --- a/apps/heft/src/cli/HeftCommandLineParser.ts +++ b/apps/heft/src/cli/HeftCommandLineParser.ts @@ -24,7 +24,7 @@ import { PhaseAction } from './actions/PhaseAction'; import { RunAction } from './actions/RunAction'; import type { IHeftActionOptions } from './actions/IHeftAction'; import { AliasAction } from './actions/AliasAction'; -import { getToolParametersFromArgs } from '../utilities/CliUtilities'; +import { getToolParameterNamesFromArgs } from '../utilities/CliUtilities'; import { Constants } from '../utilities/Constants'; /** @@ -233,7 +233,7 @@ export class HeftCommandLineParser extends CommandLineParser { throw new InternalError('onDefineParameters() has not yet been called.'); } - const toolParameters: Set = getToolParametersFromArgs(args); + const toolParameters: Set = getToolParameterNamesFromArgs(args); return { debug: toolParameters.has(this._debugFlag.longName), unmanaged: toolParameters.has(this._unmanagedFlag.longName) diff --git a/apps/heft/src/startWithVersionSelector.ts b/apps/heft/src/startWithVersionSelector.ts index 561793ec41d..3385251b258 100644 --- a/apps/heft/src/startWithVersionSelector.ts +++ b/apps/heft/src/startWithVersionSelector.ts @@ -8,7 +8,7 @@ import * as path from 'path'; import * as fs from 'fs'; import type { IPackageJson } from '@rushstack/node-core-library'; -import { getToolParametersFromArgs } from './utilities/CliUtilities'; +import { getToolParameterNamesFromArgs } from './utilities/CliUtilities'; import { Constants } from './utilities/Constants'; const HEFT_PACKAGE_NAME: string = '@rushstack/heft'; @@ -50,7 +50,7 @@ function tryGetPackageFolderFor(resolvedFileOrFolderPath: string): string | unde * Use "heft --unmanaged" to bypass this feature. */ function tryStartLocalHeft(): boolean { - const toolParameters: Set = getToolParametersFromArgs(); + const toolParameters: Set = getToolParameterNamesFromArgs(); if (toolParameters.has(Constants.unmanagedParameterLongName)) { console.log( `Bypassing the Heft version selector because ${JSON.stringify(Constants.unmanagedParameterLongName)} ` + diff --git a/apps/heft/src/utilities/CliUtilities.ts b/apps/heft/src/utilities/CliUtilities.ts index 30f1786667f..d322dc45e1f 100644 --- a/apps/heft/src/utilities/CliUtilities.ts +++ b/apps/heft/src/utilities/CliUtilities.ts @@ -2,11 +2,11 @@ // See LICENSE in the project root for license information. /** - * Parse the arguments to the tool being executed and return the tool arguments. + * Parse the arguments to the tool being executed and return the tool argument names. * * @param argv - The arguments to parse. Defaults to `process.argv`. */ -export function getToolParametersFromArgs(argv: string[] = process.argv): Set { +export function getToolParameterNamesFromArgs(argv: string[] = process.argv): Set { const toolParameters: Set = new Set(); // Skip the first two arguments, which are the path to the Node executable and the path to the Heft // entrypoint. The remaining arguments are the tool arguments. Grab them until we reach a non-"-"-prefixed From 7edbe2bc5df6ace735bf07817d4598aa98527633 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Wed, 25 Oct 2023 14:02:40 -0700 Subject: [PATCH 13/18] Feedback --- .../providers/CommandLineParameterProvider.ts | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 880bae1aded..44e4c5dc688 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -78,8 +78,6 @@ export abstract class CommandLineParameterProvider { public readonly _ambiguousParameterParserKeysByName: Map; /** @internal */ protected readonly _registeredParameterParserKeysByName: Map; - /** @internal */ - protected _parametersRegistered: boolean; private readonly _parameters: CommandLineParameter[]; private readonly _parametersByLongName: Map; @@ -88,7 +86,8 @@ export abstract class CommandLineParameterProvider { string | typeof SCOPING_PARAMETER_GROUP, argparse.ArgumentGroup >; - private _parametersProcessed: boolean; + private _parametersHaveBeenRegistered: boolean; + private _parametersHaveBeenProcessed: boolean; private _remainder: CommandLineRemainder | undefined; /** @internal */ @@ -100,8 +99,8 @@ export abstract class CommandLineParameterProvider { this._parameterGroupsByName = new Map(); this._ambiguousParameterParserKeysByName = new Map(); this._registeredParameterParserKeysByName = new Map(); - this._parametersRegistered = false; - this._parametersProcessed = false; + this._parametersHaveBeenRegistered = false; + this._parametersHaveBeenProcessed = false; } /** @@ -115,7 +114,7 @@ export abstract class CommandLineParameterProvider { * Informs the caller if the argparse data has been processed into parameters. */ public get parametersProcessed(): boolean { - return this._parametersProcessed; + return this._parametersHaveBeenProcessed; } /** @@ -413,7 +412,7 @@ export abstract class CommandLineParameterProvider { /** @internal */ public _registerDefinedParameters(existingParameterNames?: Set): void { - if (this._parametersRegistered) { + if (this._parametersHaveBeenRegistered) { // We prevent new parameters from being defined after the first call to _registerDefinedParameters, // so we can already ensure that all parameters were registered. return; @@ -481,7 +480,7 @@ export abstract class CommandLineParameterProvider { this._getArgumentParser().addArgument(argparse.Const.REMAINDER, argparseOptions); } - this._parametersRegistered = true; + this._parametersHaveBeenRegistered = true; } /** @@ -498,11 +497,11 @@ export abstract class CommandLineParameterProvider { /** @internal */ protected _processParsedData(parserOptions: ICommandLineParserOptions, data: ICommandLineParserData): void { - if (!this._parametersRegistered) { + if (!this._parametersHaveBeenRegistered) { throw new Error('Parameters have not been registered'); } - if (this._parametersProcessed) { + if (this._parametersHaveBeenProcessed) { throw new Error('Command Line Parser Data was already processed'); } @@ -600,12 +599,12 @@ export abstract class CommandLineParameterProvider { this.remainder._setValue(data[argparse.Const.REMAINDER]); } - this._parametersProcessed = true; + this._parametersHaveBeenProcessed = true; } /** @internal */ protected _defineParameter(parameter: CommandLineParameter): void { - if (this._parametersRegistered) { + if (this._parametersHaveBeenRegistered) { throw new Error('Parameters have already been registered for this provider'); } @@ -639,7 +638,7 @@ export abstract class CommandLineParameterProvider { /** @internal */ protected _defineAmbiguousParameter(name: string): string { - if (this._parametersRegistered) { + if (this._parametersHaveBeenRegistered) { throw new Error('Parameters have already been registered for this provider'); } From 85f29343b63f0376960d25c89e3a2352f1f50b85 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Wed, 25 Oct 2023 14:03:53 -0700 Subject: [PATCH 14/18] Update API --- common/reviews/api/ts-command-line.api.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index b35dae028e1..d0a49b677ec 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -183,8 +183,6 @@ export abstract class CommandLineParameterProvider { protected onDefineParameters?(): void; get parameters(): ReadonlyArray; get parametersProcessed(): boolean; - // @internal (undocumented) - protected _parametersRegistered: boolean; parseScopedLongName(scopedLongName: string): IScopedLongNameParseResult; // @internal (undocumented) protected _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; From 0a7dd4aa3ff15adf69bb86ba5b4d2916b5428f98 Mon Sep 17 00:00:00 2001 From: Daniel <3473356+D4N14L@users.noreply.github.com> Date: Fri, 27 Oct 2023 10:27:43 -0700 Subject: [PATCH 15/18] Apply suggestions from code review Co-authored-by: Ian Clanton-Thuon --- ...r-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json | 2 +- ...r-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json index 4622de5a60b..16f3f28c808 100644 --- a/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json +++ b/common/changes/@rushstack/heft/user-danade-PreventAmbiguousAbbreviations_2023-10-24-18-11.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@rushstack/heft", - "comment": "[BREAKING CHANGE] Remove \"heft run\" short-parameters for \"--to\", \"--to-except\", and \"--only\"", + "comment": "[BREAKING CHANGE] Remove \"heft run\" short-parameters for \"--to\" (\"-t\"), \"--to-except\" (\"-T\"), and \"--only\" (\"-o\").", "type": "minor" } ], diff --git a/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json index 8e1587ae251..53dfc0b7fb8 100644 --- a/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json +++ b/common/changes/@rushstack/ts-command-line/user-danade-PreventAmbiguousAbbreviations_2023-10-23-20-01.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@rushstack/ts-command-line", - "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations. For example, if a CLI tool `mytool` has a parameter `--myparam` and an action `myaction`, then `myaction` would not accept a parameter named `--myparam`. Additionally, any parameter that can be abbreviated to `--myparam` must be uniquely provided, ex. `--myparam-2` can only be abbreviated to `--myparam-`, since any shorter abbreviation would be ambiguous with the original `--myparam` on the tool.", + "comment": "Consider parent tool and action parameters when determining ambiguous abbreviations. For example, if a CLI tool `mytool` has a parameter `--myparam` and an action `myaction`, then `myaction` would not accept a parameter named `--myparam` (i.e. - `mytool --myparam myaction` is valid, `mytool myaction --myparam` is not). Additionally, any parameter that can be abbreviated to `--myparam` must be uniquely provided (i.e. - `--myparam-2` can only be abbreviated to `--myparam-`, since any shorter abbreviation would be ambiguous with the original `--myparam` on the tool).", "type": "minor" } ], From 1eca3a3366ecc5603eae163d5eb11d89696c80f8 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Fri, 27 Oct 2023 10:55:47 -0700 Subject: [PATCH 16/18] Use a required state object to contain the registration state of the parameters --- common/reviews/api/ts-command-line.api.md | 10 ++-- .../src/providers/AliasCommandLineAction.ts | 8 ++-- .../providers/CommandLineParameterProvider.ts | 30 ++++++++++-- .../src/providers/CommandLineParser.ts | 27 ++++++++--- .../src/providers/ScopedCommandLineAction.ts | 47 ++++++++++++------- .../src/test/CommandLineParser.test.ts | 2 +- .../src/test/CommandLineRemainder.test.ts | 2 +- 7 files changed, 88 insertions(+), 38 deletions(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index d0a49b677ec..ef4e902377d 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -13,8 +13,10 @@ export class AliasCommandLineAction extends CommandLineAction { protected onExecute(): Promise; // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; + // Warning: (ae-forgotten-export) The symbol "IRegisterDefinedParametersState" needs to be exported by the entry point index.d.ts + // // @internal (undocumented) - _registerDefinedParameters(existingParameterNames?: Set): void; + _registerDefinedParameters(state: IRegisterDefinedParametersState): void; readonly targetAction: CommandLineAction; } @@ -189,7 +191,7 @@ export abstract class CommandLineParameterProvider { // (undocumented) protected _registerAmbiguousParameter(name: string, parserKey: string): void; // @internal (undocumented) - _registerDefinedParameters(existingParameterNames?: Set): void; + _registerDefinedParameters(state: IRegisterDefinedParametersState): void; // @internal (undocumented) protected readonly _registeredParameterParserKeysByName: Map; // @internal (undocumented) @@ -219,7 +221,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { protected _getArgumentParser(): argparse.ArgumentParser; protected onExecute(): Promise; // @internal (undocumented) - _registerDefinedParameters(existingParameterNames?: Set): void; + _registerDefinedParameters(state: IRegisterDefinedParametersState): void; selectedAction: CommandLineAction | undefined; tryGetAction(actionName: string): CommandLineAction | undefined; } @@ -389,7 +391,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; // @internal (undocumented) - _registerDefinedParameters(existingParameterNames?: Set): void; + _registerDefinedParameters(state: IRegisterDefinedParametersState): void; static readonly ScopingParameterGroup: typeof SCOPING_PARAMETER_GROUP; } diff --git a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts index 06e90b6fd7b..c69f5f791f2 100644 --- a/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/AliasCommandLineAction.ts @@ -5,7 +5,7 @@ import * as argparse from 'argparse'; import { CommandLineAction } from './CommandLineAction'; import { CommandLineParameterKind, type CommandLineParameter } from '../parameters/BaseClasses'; -import type { ICommandLineParserData } from './CommandLineParameterProvider'; +import type { ICommandLineParserData, IRegisterDefinedParametersState } from './CommandLineParameterProvider'; import type { ICommandLineParserOptions } from './CommandLineParser'; import type { CommandLineChoiceParameter } from '../parameters/CommandLineChoiceParameter'; import type { CommandLineFlagParameter } from '../parameters/CommandLineFlagParameter'; @@ -85,7 +85,7 @@ export class AliasCommandLineAction extends CommandLineAction { } /** @internal */ - public _registerDefinedParameters(existingParameterNames?: Set): void { + public _registerDefinedParameters(state: IRegisterDefinedParametersState): void { /* override */ // All parameters are going to be defined by the target action. Re-use the target action parameters // for this action. @@ -153,8 +153,8 @@ export class AliasCommandLineAction extends CommandLineAction { // Finally, register the parameters with the parser. We need to make sure that the target action // is registered, since we need to re-use its parameters, and ambiguous parameters are discovered // during registration. This will no-op if the target action is already registered. - this.targetAction._registerDefinedParameters(existingParameterNames); - super._registerDefinedParameters(existingParameterNames); + this.targetAction._registerDefinedParameters(state); + super._registerDefinedParameters(state); // We need to re-map the ambiguous parameters after they are defined by calling // super._registerDefinedParameters() diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 44e4c5dc688..6ed515e2501 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -49,6 +49,19 @@ export interface IScopedLongNameParseResult { scope: string | undefined; } +/** + * An object containing the state of the + * + * @public + */ +export interface IRegisterDefinedParametersState { + /** + * A set of all defined parameter names registered by parent {@link CommandLineParameterProvider} + * objects. + */ + parentParameterNames: Set; +} + /** * This is the argparse result data object * @internal @@ -344,7 +357,10 @@ export abstract class CommandLineParameterProvider { * Generates the command-line help text. */ public renderHelpText(): string { - this._registerDefinedParameters(); + const initialState: IRegisterDefinedParametersState = { + parentParameterNames: new Set() + }; + this._registerDefinedParameters(initialState); return this._getArgumentParser().formatHelp(); } @@ -352,7 +368,10 @@ export abstract class CommandLineParameterProvider { * Generates the command-line usage text. */ public renderUsageText(): string { - this._registerDefinedParameters(); + const initialState: IRegisterDefinedParametersState = { + parentParameterNames: new Set() + }; + this._registerDefinedParameters(initialState); return this._getArgumentParser().formatUsage(); } @@ -411,7 +430,7 @@ export abstract class CommandLineParameterProvider { } /** @internal */ - public _registerDefinedParameters(existingParameterNames?: Set): void { + public _registerDefinedParameters(state: IRegisterDefinedParametersState): void { if (this._parametersHaveBeenRegistered) { // We prevent new parameters from being defined after the first call to _registerDefinedParameters, // so we can already ensure that all parameters were registered. @@ -454,8 +473,9 @@ export abstract class CommandLineParameterProvider { // Register the existing parameters as ambiguous parameters. These are generally provided by the // parent action. - for (const existingParameterName of existingParameterNames || []) { - this._defineAmbiguousParameter(existingParameterName); + const { parentParameterNames } = state; + for (const parentParameterName of parentParameterNames) { + this._defineAmbiguousParameter(parentParameterName); } // We also need to loop through the defined ambiguous parameters and register them. These will be reported diff --git a/libraries/ts-command-line/src/providers/CommandLineParser.ts b/libraries/ts-command-line/src/providers/CommandLineParser.ts index b6af2e7fa1b..bfae5a4b7da 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParser.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParser.ts @@ -6,7 +6,11 @@ import colors from 'colors'; import type { CommandLineAction } from './CommandLineAction'; import type { AliasCommandLineAction } from './AliasCommandLineAction'; -import { CommandLineParameterProvider, type ICommandLineParserData } from './CommandLineParameterProvider'; +import { + CommandLineParameterProvider, + type IRegisterDefinedParametersState, + type ICommandLineParserData +} from './CommandLineParameterProvider'; import { CommandLineParserExitError, CustomArgumentParser } from './CommandLineParserExitError'; import { TabCompleteAction } from './TabCompletionAction'; @@ -202,7 +206,10 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { this._validateDefinitions(); // Register the parameters before we print help or parse the CLI - this._registerDefinedParameters(); + const initialState: IRegisterDefinedParametersState = { + parentParameterNames: new Set() + }; + this._registerDefinedParameters(initialState); if (!args) { // 0=node.exe, 1=script name @@ -262,15 +269,21 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { } /** @internal */ - public _registerDefinedParameters(existingParameterNames?: Set): void { - super._registerDefinedParameters(existingParameterNames); + public _registerDefinedParameters(state: IRegisterDefinedParametersState): void { + super._registerDefinedParameters(state); - const registeredParameterNames: Set = new Set([ - ...(existingParameterNames || []), + const { parentParameterNames } = state; + const updatedParentParameterNames: Set = new Set([ + ...parentParameterNames, ...this._registeredParameterParserKeysByName.keys() ]); + + const parentState: IRegisterDefinedParametersState = { + ...state, + parentParameterNames: updatedParentParameterNames + }; for (const action of this._actions) { - action._registerDefinedParameters(registeredParameterNames); + action._registerDefinedParameters(parentState); } } diff --git a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts index 8786be4bd7e..561eb5de428 100644 --- a/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts +++ b/libraries/ts-command-line/src/providers/ScopedCommandLineAction.ts @@ -6,7 +6,11 @@ import { CommandLineAction, type ICommandLineActionOptions } from './CommandLine import { CommandLineParser, type ICommandLineParserOptions } from './CommandLineParser'; import { CommandLineParserExitError } from './CommandLineParserExitError'; import type { CommandLineParameter } from '../parameters/BaseClasses'; -import type { CommandLineParameterProvider, ICommandLineParserData } from './CommandLineParameterProvider'; +import type { + CommandLineParameterProvider, + ICommandLineParserData, + IRegisterDefinedParametersState +} from './CommandLineParameterProvider'; interface IInternalScopedCommandLineParserOptions extends ICommandLineParserOptions { readonly actionOptions: ICommandLineActionOptions; @@ -14,7 +18,7 @@ interface IInternalScopedCommandLineParserOptions extends ICommandLineParserOpti readonly onDefineScopedParameters: (commandLineParameterProvider: CommandLineParameterProvider) => void; readonly aliasAction?: string; readonly aliasDocumentation?: string; - readonly existingParameterNames?: Set; + readonly registerDefinedParametersState: IRegisterDefinedParametersState; } /** @@ -59,8 +63,10 @@ class InternalScopedCommandLineParser extends CommandLineParser { this._internalOptions.onDefineScopedParameters(this); } - public _registerDefinedParameters(): void { - super._registerDefinedParameters(this._internalOptions.existingParameterNames); + public _registerDefinedParameters(state: IRegisterDefinedParametersState): void { + // Since we are in a separate parser, we need to register the parameters using the state + // from the parent parser. + super._registerDefinedParameters(this._internalOptions.registerDefinedParametersState); } protected async onExecute(): Promise { @@ -97,7 +103,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { private _scopingParameters: CommandLineParameter[]; private _unscopedParserOptions: ICommandLineParserOptions | undefined; private _scopedCommandLineParser: InternalScopedCommandLineParser | undefined; - private _existingParameterNames: Set = new Set(); + private _subparserState: IRegisterDefinedParametersState | undefined; /** * The required group name to apply to all scoping parameters. At least one parameter @@ -131,6 +137,12 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { // override super._processParsedData(parserOptions, data); + // This should never happen because the super method should throw if parameters haven't been registered, + // but guard against this just in-case. + if (this._subparserState === undefined) { + throw new Error('Parameters have not been registered'); + } + this._unscopedParserOptions = parserOptions; // Generate the scoped parser using the parent parser information. We can only create this after we @@ -141,7 +153,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { aliasAction: data.aliasAction, aliasDocumentation: data.aliasDocumentation, unscopedActionParameters: this.parameters, - existingParameterNames: this._existingParameterNames, + registerDefinedParametersState: this._subparserState, onDefineScopedParameters: this.onDefineScopedParameters.bind(this) }); } @@ -190,16 +202,19 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { } /** @internal */ - public _registerDefinedParameters(existingParameterNames?: Set): void { - super._registerDefinedParameters(existingParameterNames); - - for (const registeredParameterName of this._registeredParameterParserKeysByName.keys()) { - this._existingParameterNames.add(registeredParameterName); - } - - for (const existingParameterName of existingParameterNames || []) { - this._existingParameterNames.add(existingParameterName); - } + public _registerDefinedParameters(state: IRegisterDefinedParametersState): void { + super._registerDefinedParameters(state); + + const { parentParameterNames } = state; + const updatedParentParameterNames: Set = new Set([ + ...parentParameterNames, + ...this._registeredParameterParserKeysByName.keys() + ]); + + this._subparserState = { + ...state, + parentParameterNames: updatedParentParameterNames + }; } /** diff --git a/libraries/ts-command-line/src/test/CommandLineParser.test.ts b/libraries/ts-command-line/src/test/CommandLineParser.test.ts index 2e3029f8847..7c63f6dbffa 100644 --- a/libraries/ts-command-line/src/test/CommandLineParser.test.ts +++ b/libraries/ts-command-line/src/test/CommandLineParser.test.ts @@ -48,7 +48,7 @@ class TestCommandLine extends CommandLineParser { describe(CommandLineParser.name, () => { it('executes an action', async () => { const commandLineParser: TestCommandLine = new TestCommandLine(); - commandLineParser._registerDefinedParameters(); + commandLineParser._registerDefinedParameters({ parentParameterNames: new Set() }); await commandLineParser.execute(['do:the-job', '--flag']); diff --git a/libraries/ts-command-line/src/test/CommandLineRemainder.test.ts b/libraries/ts-command-line/src/test/CommandLineRemainder.test.ts index a42d12d363d..e0cc2d2a6f4 100644 --- a/libraries/ts-command-line/src/test/CommandLineRemainder.test.ts +++ b/libraries/ts-command-line/src/test/CommandLineRemainder.test.ts @@ -37,7 +37,7 @@ function createParser(): DynamicCommandLineParser { description: 'The action remainder' }); - commandLineParser._registerDefinedParameters(); + commandLineParser._registerDefinedParameters({ parentParameterNames: new Set() }); return commandLineParser; } From aebf052653d60567da86fa72a245aaf56a494340 Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 30 Oct 2023 14:19:08 -0700 Subject: [PATCH 17/18] Update exports --- libraries/ts-command-line/src/index.ts | 3 ++- .../src/providers/CommandLineParameterProvider.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/ts-command-line/src/index.ts b/libraries/ts-command-line/src/index.ts index 693d787cdcd..50305a59246 100644 --- a/libraries/ts-command-line/src/index.ts +++ b/libraries/ts-command-line/src/index.ts @@ -43,7 +43,8 @@ export { CommandLineRemainder } from './parameters/CommandLineRemainder'; export { CommandLineParameterProvider, IScopedLongNameParseResult, - ICommandLineParserData as _ICommandLineParserData + ICommandLineParserData as _ICommandLineParserData, + IRegisterDefinedParametersState as _IRegisterDefinedParametersState } from './providers/CommandLineParameterProvider'; export { ICommandLineParserOptions, CommandLineParser } from './providers/CommandLineParser'; diff --git a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts index 6ed515e2501..1e64f578c80 100644 --- a/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts +++ b/libraries/ts-command-line/src/providers/CommandLineParameterProvider.ts @@ -52,7 +52,7 @@ export interface IScopedLongNameParseResult { /** * An object containing the state of the * - * @public + * @internal */ export interface IRegisterDefinedParametersState { /** From 1a385b4a5b315e39a9203aa66f0aa2bc38d6232f Mon Sep 17 00:00:00 2001 From: Daniel Nadeau <3473356+D4N14L@users.noreply.github.com> Date: Mon, 30 Oct 2023 14:23:09 -0700 Subject: [PATCH 18/18] Use type exports and update api json --- common/reviews/api/ts-command-line.api.md | 15 +++++++++------ libraries/ts-command-line/src/index.ts | 17 ++++++++++------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/common/reviews/api/ts-command-line.api.md b/common/reviews/api/ts-command-line.api.md index ef4e902377d..5cf20bde5d8 100644 --- a/common/reviews/api/ts-command-line.api.md +++ b/common/reviews/api/ts-command-line.api.md @@ -13,10 +13,8 @@ export class AliasCommandLineAction extends CommandLineAction { protected onExecute(): Promise; // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; - // Warning: (ae-forgotten-export) The symbol "IRegisterDefinedParametersState" needs to be exported by the entry point index.d.ts - // // @internal (undocumented) - _registerDefinedParameters(state: IRegisterDefinedParametersState): void; + _registerDefinedParameters(state: _IRegisterDefinedParametersState): void; readonly targetAction: CommandLineAction; } @@ -191,7 +189,7 @@ export abstract class CommandLineParameterProvider { // (undocumented) protected _registerAmbiguousParameter(name: string, parserKey: string): void; // @internal (undocumented) - _registerDefinedParameters(state: IRegisterDefinedParametersState): void; + _registerDefinedParameters(state: _IRegisterDefinedParametersState): void; // @internal (undocumented) protected readonly _registeredParameterParserKeysByName: Map; // @internal (undocumented) @@ -221,7 +219,7 @@ export abstract class CommandLineParser extends CommandLineParameterProvider { protected _getArgumentParser(): argparse.ArgumentParser; protected onExecute(): Promise; // @internal (undocumented) - _registerDefinedParameters(state: IRegisterDefinedParametersState): void; + _registerDefinedParameters(state: _IRegisterDefinedParametersState): void; selectedAction: CommandLineAction | undefined; tryGetAction(actionName: string): CommandLineAction | undefined; } @@ -368,6 +366,11 @@ export interface ICommandLineStringDefinition extends IBaseCommandLineDefinition export interface ICommandLineStringListDefinition extends IBaseCommandLineDefinitionWithArgument { } +// @internal +export interface _IRegisterDefinedParametersState { + parentParameterNames: Set; +} + // @public export interface IScopedLongNameParseResult { longName: string; @@ -391,7 +394,7 @@ export abstract class ScopedCommandLineAction extends CommandLineAction { // @internal _processParsedData(parserOptions: ICommandLineParserOptions, data: _ICommandLineParserData): void; // @internal (undocumented) - _registerDefinedParameters(state: IRegisterDefinedParametersState): void; + _registerDefinedParameters(state: _IRegisterDefinedParametersState): void; static readonly ScopingParameterGroup: typeof SCOPING_PARAMETER_GROUP; } diff --git a/libraries/ts-command-line/src/index.ts b/libraries/ts-command-line/src/index.ts index 50305a59246..42c4130de1b 100644 --- a/libraries/ts-command-line/src/index.ts +++ b/libraries/ts-command-line/src/index.ts @@ -7,12 +7,15 @@ * @packageDocumentation */ -export { CommandLineAction, ICommandLineActionOptions } from './providers/CommandLineAction'; +export { CommandLineAction, type ICommandLineActionOptions } from './providers/CommandLineAction'; export { DynamicCommandLineAction } from './providers/DynamicCommandLineAction'; export { ScopedCommandLineAction } from './providers/ScopedCommandLineAction'; -export { AliasCommandLineAction, IAliasCommandLineActionOptions } from './providers/AliasCommandLineAction'; - export { + AliasCommandLineAction, + type IAliasCommandLineActionOptions +} from './providers/AliasCommandLineAction'; + +export type { IBaseCommandLineDefinition, IBaseCommandLineDefinitionWithArgument, ICommandLineFlagDefinition, @@ -42,12 +45,12 @@ export { CommandLineRemainder } from './parameters/CommandLineRemainder'; export { CommandLineParameterProvider, - IScopedLongNameParseResult, - ICommandLineParserData as _ICommandLineParserData, - IRegisterDefinedParametersState as _IRegisterDefinedParametersState + type IScopedLongNameParseResult, + type ICommandLineParserData as _ICommandLineParserData, + type IRegisterDefinedParametersState as _IRegisterDefinedParametersState } from './providers/CommandLineParameterProvider'; -export { ICommandLineParserOptions, CommandLineParser } from './providers/CommandLineParser'; +export { CommandLineParser, type ICommandLineParserOptions } from './providers/CommandLineParser'; export { DynamicCommandLineParser } from './providers/DynamicCommandLineParser'; export { CommandLineConstants } from './Constants';