From f9c491fb0620995c5a91fe759eab19d23c4ffaa2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 03:02:01 +0000 Subject: [PATCH 1/7] Convert CLI option parsing from yargs-parser to commander - Replace yargs-parser with commander package (v10, Node 14+ compatible) - Remove yargs-parser and @types/yargs-parser dependencies - Ban unknown options (commander rejects unrecognized options) - Remove --bool=false and --bool false support in favor of --no-bool - Duplicate non-array options now use last-write-wins instead of throwing - CamelCase input (--gitTags) no longer accepted; use hyphenated (--git-tags) - Update tests to match new behavior - All existing options, aliases, and commands preserved Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/b10c3147-797f-4524-a56d-1f2537833d47 --- package.json | 5 +- .../options/getCliOptions.test.ts | 64 ++--- .../listPackageVersions.test.ts | 2 +- src/options/getCliOptions.ts | 269 +++++++++--------- yarn.lock | 9 +- 5 files changed, 160 insertions(+), 189 deletions(-) diff --git a/package.json b/package.json index be8404f1f..c0a4acf28 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,7 @@ }, "dependencies": { "@vercel/detect-agent": "^1.2.1", + "commander": "^10.0.1", "cosmiconfig": "^9.0.0", "execa": "^5.0.0", "minimatch": "^3.0.4", @@ -60,8 +61,7 @@ "p-limit": "^3.0.2", "prompts": "^2.4.2", "semver": "^7.0.0", - "workspace-tools": "^0.41.0", - "yargs-parser": "^21.0.0" + "workspace-tools": "^0.41.0" }, "devDependencies": { "@jest/globals": "^29.0.0", @@ -70,7 +70,6 @@ "@types/prompts": "^2.4.2", "@types/semver": "^7.3.13", "@types/tmp": "^0.2.3", - "@types/yargs-parser": "^21.0.0", "@typescript-eslint/eslint-plugin": "^5.0.0", "@typescript-eslint/parser": "^5.0.0", "@typescript-eslint/utils": "^5.0.0", diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index 09b005c0e..946a45911 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -11,8 +11,7 @@ jest.mock('workspace-tools', () => { // // These tests cover a mix of built-in parser behavior, provided options, and custom overrides. -// It's worth having tests for relevant built-in behaviors in case we change parsers in the future -// (likely to commander), to ensure there are no undocumented breaking changes from the beachball +// The parser is commander, and these tests document the expected behavior from the beachball // "end user" perspective. // describe('getCliOptions', () => { @@ -76,8 +75,9 @@ describe('getCliOptions', () => { expect(options).toEqual({ ...defaults, scope: ['a,b', 'c,d'] }); }); - it('throws if non-array option is specified multiple times', () => { - expect(() => getCliOptionsTest(['--tag', 'foo', '--tag', 'baz'])).toThrow(); + it('uses last value if non-array option is specified multiple times', () => { + const options = getCliOptionsTest(['--tag', 'foo', '--tag', 'baz']); + expect(options).toEqual({ ...defaults, tag: 'baz' }); }); it('parses negated boolean option', () => { @@ -85,20 +85,9 @@ describe('getCliOptions', () => { expect(options).toEqual({ ...defaults, fetch: false }); }); - it('parses valid boolean option values', () => { - const falseOptions = getCliOptionsTest(['--fetch=false', '--yes', 'false']); - expect(falseOptions).toEqual({ ...defaults, fetch: false, yes: false }); - - const trueOptions = getCliOptionsTest(['--fetch=true', '--yes', 'true']); - expect(trueOptions).toEqual({ ...defaults, fetch: true, yes: true }); - }); - - it('parses boolean flag with valid value', () => { - const falseOptions = getCliOptionsTest(['-y', 'false']); - expect(falseOptions).toEqual({ ...defaults, yes: false }); - - const trueOptions = getCliOptionsTest(['-y', 'true']); - expect(trueOptions).toEqual({ ...defaults, yes: true }); + it('parses negated boolean options with --no-X syntax', () => { + const options = getCliOptionsTest(['--no-fetch', '--no-yes']); + expect(options).toEqual({ ...defaults, fetch: false, yes: false }); }); it('throws on invalid numeric value', () => { @@ -110,10 +99,10 @@ describe('getCliOptions', () => { expect(options).toEqual({ ...defaults, gitTags: true, dependentChangeType: 'patch' }); }); - it('supports camel case for options defined as hyphenated', () => { + it('requires hyphenated form for multi-word options', () => { const options = getCliOptionsTest([ - '--gitTags', - '--dependentChangeType', + '--git-tags', + '--dependent-change-type', 'patch', '--disallowed-change-types', 'major', @@ -175,37 +164,28 @@ describe('getCliOptions', () => { expect(getDefaultRemoteBranch).toHaveBeenCalledWith({ branch: 'foo', verbose: undefined, cwd: projectRoot }); }); - it('preserves additional string options', () => { - const options = getCliOptionsTest(['--foo', 'bar', '--baz=qux']); - expect(options).toEqual({ ...defaults, foo: 'bar', baz: 'qux' }); + it('throws on unknown string options', () => { + expect(() => getCliOptionsTest(['--foo', 'bar'])).toThrow(); }); - it('handles additional boolean flags as booleans', () => { - const options = getCliOptionsTest(['--foo', '--no-bar']); - expect(options).toEqual({ ...defaults, foo: true, bar: false }); + it('throws on unknown boolean flags', () => { + expect(() => getCliOptionsTest(['--foo'])).toThrow(); }); - it('handles additional boolean text values as booleans', () => { - const options = getCliOptionsTest(['--foo', 'true', '--bar=false']); - expect(options).toEqual({ ...defaults, foo: true, bar: false }); + it('throws on unknown negated boolean flags', () => { + expect(() => getCliOptionsTest(['--no-bar'])).toThrow(); }); - it('handles additional numeric values as numbers', () => { - const options = getCliOptionsTest(['--foo', '1', '--bar=2']); - expect(options).toEqual({ ...defaults, foo: 1, bar: 2 }); + it('throws on unknown option with value', () => { + expect(() => getCliOptionsTest(['--foo', 'true'])).toThrow(); }); - it('handles additional option specified multiple times as array', () => { - const options = getCliOptionsTest(['--foo', 'bar', '--foo', 'baz']); - expect(options).toEqual({ ...defaults, foo: ['bar', 'baz'] }); + it('throws on unknown option specified multiple times', () => { + expect(() => getCliOptionsTest(['--foo', 'bar', '--foo', 'baz'])).toThrow(); }); - // documenting current behavior (doesn't have to stay this way) - it('for additional options, does not handle multiple values as part of array', () => { - // in this case the trailing value "baz" would be treated as the command since it's the first - // positional option - const options = getCliOptionsTest(['--foo', 'bar', 'baz']); - expect(options).toEqual({ ...defaults, foo: 'bar', command: 'baz' }); + it('throws on unknown option followed by positional', () => { + expect(() => getCliOptionsTest(['--foo', 'bar', 'baz'])).toThrow(); }); describe('config command', () => { diff --git a/src/__tests__/packageManager/listPackageVersions.test.ts b/src/__tests__/packageManager/listPackageVersions.test.ts index d8b9e4fc8..9383284d0 100644 --- a/src/__tests__/packageManager/listPackageVersions.test.ts +++ b/src/__tests__/packageManager/listPackageVersions.test.ts @@ -318,7 +318,7 @@ describe('list npm versions', () => { npmMock.setRegistryData({ foo: { 'dist-tags': { latest: '1.0.0', beta: '2.0.0-beta' } } }); const { packages, options } = getOptionsAndPackages({ packages: { foo: {} }, - extraArgv: ['--authType', 'password', '--token', 'pass'], + extraArgv: ['--auth-type', 'password', '--token', 'pass'], }); expect(options).toMatchObject({ authType: 'password', token: 'pass' }); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index 37c8e020f..06e7e8ba4 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -1,5 +1,5 @@ -import parser from 'yargs-parser'; -import type { CliOptions, ParsedOptions } from '../types/BeachballOptions'; +import { Command, Option } from 'commander'; +import type { ParsedOptions } from '../types/BeachballOptions'; import { getDefaultRemoteBranch, findProjectRoot } from 'workspace-tools'; import { env } from '../env'; @@ -15,109 +15,99 @@ export interface ProcessInfo { cwd: string; } -// For camelCased options, yargs will automatically accept them with-dashes too. -const arrayOptions = ['disallowedChangeTypes', 'package', 'scope'] as const; -const booleanOptions = [ - 'all', - 'bump', - 'bumpDeps', - 'commit', - 'disallowDeletedChangeFiles', - 'fetch', - 'forceVersions', - 'gitTags', - 'help', - 'keepChangeFiles', - 'new', - 'publish', - 'push', - 'verbose', - 'version', - 'yes', -] as const; -const numberOptions = ['concurrency', 'depth', 'npmReadConcurrency', 'gitTimeout', 'retries', 'timeout'] as const; -const stringOptions = [ - 'access', - 'authType', - 'branch', - 'canaryName', - 'changehint', - 'changeDir', - 'configPath', - 'dependentChangeType', - 'fromRef', - 'message', - 'packToPath', - 'prereleasePrefix', - 'registry', - 'tag', - 'token', - 'type', -] as const; - -type AtLeastOne = [T, ...T[]]; -/** Type hack to verify that an array includes all keys of a type */ -const allKeysOfType = - () => - >( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ...x: L extends any ? (Exclude extends never ? L : Exclude[]) : never - ) => - x; - -// Verify that all the known CLI options have types specified, to ensure correct parsing. -// -// NOTE: If a prop is missing, this will have a somewhat misleading error: -// Argument of type '"disallowedChangeTypes"' is not assignable to parameter of type '"tag" | "version"' -// -// To fix, add the missing names after "parameter of type" ("tag" and "version" in this example) -// to the appropriate array above. -const knownOptions = allKeysOfType()( - ...arrayOptions, - ...booleanOptions, - ...numberOptions, - ...stringOptions, - // these options are filled in below, not respected from the command line - 'path', - 'command', - '_extraPositionalArgs' -); - -const parserOptions: parser.Options = { - configuration: { - 'boolean-negation': true, - 'camel-case-expansion': true, - 'dot-notation': false, - 'duplicate-arguments-array': true, - 'flatten-duplicate-arrays': true, - 'greedy-arrays': true, // for now; we might want to change this to false in the future - 'parse-numbers': true, - 'parse-positional-numbers': false, - 'short-option-groups': false, - 'strip-aliased': true, - 'strip-dashed': true, - }, - // spread to get rid of readonly... - array: [...arrayOptions], - boolean: [...booleanOptions], - number: [...numberOptions], - string: [...stringOptions], - alias: { - authType: ['a'], - branch: ['b'], - configPath: ['c', 'config'], - forceVersions: ['force'], - fromRef: ['since'], - help: ['h', '?'], - message: ['m'], - package: ['p'], - registry: ['r'], - tag: ['t'], - token: ['n'], - version: ['v'], - yes: ['y'], - }, -}; +/** Parse a string value as a number, throwing if the result is NaN. */ +function parseNumber(value: string, optionName: string): number { + const num = Number(value); + if (isNaN(num)) { + throw new Error(`Non-numeric value passed for numeric option "${optionName}"`); + } + return num; +} + +/** + * Creates and configures the commander program with all beachball options. + */ +function createProgram(): Command { + const program = new Command(); + program + .allowExcessArguments(true) + .exitOverride() // throw instead of calling process.exit + .configureOutput({ + // eslint-disable-next-line @typescript-eslint/no-empty-function + writeOut: () => {}, // suppress commander's built-in output + // eslint-disable-next-line @typescript-eslint/no-empty-function + writeErr: () => {}, + }); + + // Disable commander's built-in --help and --version so we can handle them ourselves + program.helpOption(false); + + // -- Boolean options (with --no-X negation support) -- + program.option('--all'); + program.option('--bump'); + program.option('--no-bump'); + program.option('--bump-deps'); + program.option('--no-bump-deps'); + program.option('--commit'); + program.option('--no-commit'); + program.option('--disallow-deleted-change-files'); + program.option('--no-disallow-deleted-change-files'); + program.option('--fetch'); + program.option('--no-fetch'); + program.option('--force, --force-versions'); + program.option('--git-tags'); + program.option('--no-git-tags'); + program.option('-h, --help'); + program.option('--keep-change-files'); + program.option('--no-keep-change-files'); + program.option('--new'); + program.option('--no-new'); + program.option('--publish'); + program.option('--no-publish'); + program.option('--push'); + program.option('--no-push'); + program.option('--verbose'); + program.option('-v, --version'); + program.option('-y, --yes'); + program.option('--no-yes'); + + // -- Array options (variadic: accepts multiple values or repeated flags) -- + program.option('--disallowed-change-types '); + program.option('-p, --package '); + program.option('--scope '); + + // -- Number options -- + program.addOption(new Option('--concurrency ').argParser((v: string) => parseNumber(v, 'concurrency'))); + program.addOption(new Option('--depth ').argParser((v: string) => parseNumber(v, 'depth'))); + program.addOption( + new Option('--npm-read-concurrency ').argParser((v: string) => parseNumber(v, 'npmReadConcurrency')) + ); + program.addOption(new Option('--git-timeout ').argParser((v: string) => parseNumber(v, 'gitTimeout'))); + program.addOption(new Option('--retries ').argParser((v: string) => parseNumber(v, 'retries'))); + program.addOption(new Option('--timeout ').argParser((v: string) => parseNumber(v, 'timeout'))); + + // -- String options -- + program.option('--access '); + program.option('-a, --auth-type '); + program.option('-b, --branch '); + program.option('--canary-name '); + program.option('--changehint '); + program.option('--change-dir '); + program.option('-c, --config-path '); + // --config is a long alias for --config-path (commander only supports one long name per option) + program.addOption(new Option('--config ').hideHelp()); + program.option('--dependent-change-type '); + program.option('--since, --from-ref '); + program.option('-m, --message '); + program.option('--pack-to-path '); + program.option('--prerelease-prefix '); + program.option('-r, --registry '); + program.option('-t, --tag '); + program.option('-n, --token '); + program.option('--type '); + + return program; +} /** * Gets CLI options. @@ -131,12 +121,12 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti { argv: processOrArgv, cwd: env.isJest ? '' : process.cwd() } : processOrArgv; - // Be careful not to mutate the input argv - const trimmedArgv = processInfo.argv.slice(2); + const program = createProgram(); + program.parse(processInfo.argv); - const args = parser(trimmedArgv, parserOptions); + const commanderOpts = program.opts(); + const positionalArgs = program.args; - const { _: positionalArgs, ...options } = args; let cwd = processInfo.cwd; try { // If a non-empty cwd is provided, find the project root from there. @@ -148,21 +138,38 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti // use the provided cwd } - if (positionalArgs.length > 1 && String(positionalArgs[0]) !== 'config') { + // Determine command from positional args + const command = positionalArgs.length ? String(positionalArgs[0]) : 'change'; + + if (positionalArgs.length > 1 && command !== 'config') { throw new Error(`Only one positional argument (the command) is allowed. Received: ${positionalArgs.join(' ')}`); } + // Build the cliOptions object from commander's parsed options. + // Only include options that were explicitly set on the command line (not undefined defaults). const cliOptions: ParsedOptions['cliOptions'] = { - ...options, - command: positionalArgs.length ? String(positionalArgs[0]) : 'change', + command, path: cwd, }; - // Save extra positional args for commands that support subcommands (e.g. 'config get ') - // (yargs-parser doesn't support positional arguments directly) - const extraPositionalArgs = positionalArgs.length > 1 ? positionalArgs.slice(1).map(String) : undefined; + // Handle --config as alias for --config-path + if (commanderOpts.config !== undefined && commanderOpts.configPath === undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + commanderOpts.configPath = commanderOpts.config; + } + delete commanderOpts.config; + + // Copy all defined options from commander to cliOptions. + // Commander already converts hyphenated option names to camelCase in its opts() output. + for (const [key, value] of Object.entries(commanderOpts)) { + if (value !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment + (cliOptions as any)[key] = value; + } + } - const branchArg = args.branch as string | undefined; + // Handle branch argument: add remote if missing slash + const branchArg = cliOptions.branch; if (branchArg) { // TODO: This logic assumes the first segment of any branch name with a slash must be the remote, // which is not necessarily accurate. Ideally we should check if a remote with that name exists, @@ -170,37 +177,17 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti cliOptions.branch = branchArg.indexOf('/') > -1 ? branchArg - : getDefaultRemoteBranch({ branch: branchArg, verbose: args.verbose as boolean | undefined, cwd }); + : getDefaultRemoteBranch({ branch: branchArg, verbose: cliOptions.verbose, cwd }); } + // For canary command, set tag to canaryName or default 'canary' if (cliOptions.command === 'canary') { cliOptions.tag = cliOptions.canaryName || 'canary'; } - for (const key of Object.keys(cliOptions) as (keyof CliOptions)[]) { - const value = cliOptions[key]; - if (value === undefined) { - delete cliOptions[key]; - } else if (typeof value === 'number' && isNaN(value)) { - throw new Error(`Non-numeric value passed for numeric option "${key}"`); - } else if (knownOptions.includes(key)) { - if (Array.isArray(value) && !arrayOptions.includes(key as (typeof arrayOptions)[number])) { - throw new Error(`Option "${key}" only accepts a single value. Received: ${value.join(' ')}`); - } - } else if (value === 'true') { - // For unknown arguments like --foo=true or --bar=false, yargs will handle the value as a string. - // Convert it to a boolean to avoid subtle bugs. - // eslint-disable-next-line - (cliOptions as any)[key] = true; - } else if (value === 'false') { - // eslint-disable-next-line - (cliOptions as any)[key] = false; - } - } - - // Set extra positional args after the validation loop (it's an internal array, not from CLI parsing) - if (extraPositionalArgs) { - cliOptions._extraPositionalArgs = extraPositionalArgs; + // Save extra positional args for commands that support subcommands (e.g. 'config get ') + if (positionalArgs.length > 1) { + cliOptions._extraPositionalArgs = positionalArgs.slice(1).map(String); } return cliOptions; diff --git a/yarn.lock b/yarn.lock index 3c854fc17..7b3f71766 100644 --- a/yarn.lock +++ b/yarn.lock @@ -758,7 +758,7 @@ resolved "https://registry.yarnpkg.com/@types/tmp/-/tmp-0.2.6.tgz#d785ee90c52d7cc020e249c948c36f7b32d1e217" integrity sha512-chhaNf2oKHlRkDGt+tiKE2Z5aJ6qalm7Z9rlLdBwmOiAAf09YQvvoLXjWK4HWPF1xU/fqvMgfNfpVoBscA/tKA== -"@types/yargs-parser@*", "@types/yargs-parser@^21.0.0": +"@types/yargs-parser@*": version "21.0.3" resolved "https://registry.yarnpkg.com/@types/yargs-parser/-/yargs-parser-21.0.3.tgz#815e30b786d2e8f0dcd85fd5bcf5e1a04d008f15" integrity sha512-I4q9QU9MQv4oEOz4tAHJtNz1cwuLxn2F3xcc2iV5WdqLPpUnj30aUuxt1mAxYTG+oe8CZMV/+6rU4S4gRDzqtQ== @@ -1621,6 +1621,11 @@ combined-stream@^1.0.6, combined-stream@~1.0.6: dependencies: delayed-stream "~1.0.0" +commander@^10.0.1: + version "10.0.1" + resolved "https://registry.yarnpkg.com/commander/-/commander-10.0.1.tgz#881ee46b4f77d1c1dccc5823433aa39b022cbe06" + integrity sha512-y4Mg2tXshplEbSGzx7amzPwKKOCGuoSRP/CjEdwwk0FOGlUbq6lKuoyDZTNZkmxHdJtp54hdfY/JUrdL7Xfdug== + commander@^9.3.0: version "9.5.0" resolved "https://registry.yarnpkg.com/commander/-/commander-9.5.0.tgz#bc08d1eb5cedf7ccb797a96199d41c7bc3e60d30" @@ -5165,7 +5170,7 @@ yargs-parser@^20.2.2: resolved "https://registry.yarnpkg.com/yargs-parser/-/yargs-parser-20.2.9.tgz#2eb7dc3b0289718fc295f362753845c41a0c94ee" integrity sha512-y11nGElTIV+CT3Zv9t7VKl+Q3hTQoT9a1Qzezhhl6Rp21gJ/IVTW7Z3y9EWXhuUBC2Shnf+DX0antecpAwSP8w== -yargs-parser@^21.0.0, yargs-parser@^21.1.1: +yargs-parser@^21.1.1: version "21.1.1" resolved "https://registry.yarnpkg.com/yargs-parser/-/yargs-parser-21.1.1.tgz#9096bceebf990d21bb31fa9516e0ede294a77d35" integrity sha512-tVpsJW7DdjecAiFpbIB1e3qxIQsE6NoPc5/eTdrbbIC4h0LVsWhnoa3g+m2HclBIujHzsxZ4VJVA+GUuc2/LBw== From cdd3f2ee569a19cef1de84f89d07027861ecf99e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 03:03:20 +0000 Subject: [PATCH 2/7] Address code review: add config conflict error and camelCase rejection test Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/b10c3147-797f-4524-a56d-1f2537833d47 --- src/__functional__/options/getCliOptions.test.ts | 6 ++++++ src/options/getCliOptions.ts | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index 946a45911..88560e4b6 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -116,6 +116,12 @@ describe('getCliOptions', () => { }); }); + it('rejects camelCase form of multi-word options', () => { + expect(() => getCliOptionsTest(['--gitTags'])).toThrow(); + expect(() => getCliOptionsTest(['--dependentChangeType', 'patch'])).toThrow(); + expect(() => getCliOptionsTest(['--disallowedChangeTypes', 'major'])).toThrow(); + }); + it('parses short option aliases', () => { const options = getCliOptionsTest(['publish', '-t', 'test', '-r', 'http://whatever', '-y']); expect(options).toEqual({ ...defaults, command: 'publish', tag: 'test', registry: 'http://whatever', yes: true }); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index 06e7e8ba4..d02874755 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -153,7 +153,10 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti }; // Handle --config as alias for --config-path - if (commanderOpts.config !== undefined && commanderOpts.configPath === undefined) { + if (commanderOpts.config !== undefined) { + if (commanderOpts.configPath !== undefined) { + throw new Error('Cannot specify both --config and --config-path'); + } // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment commanderOpts.configPath = commanderOpts.config; } From 7ac5cacf9e349e81c5d2718343d4556b167403cf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 06:17:42 +0000 Subject: [PATCH 3/7] plan Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/637df71a-fa66-490b-98ee-ab96f3469c64 --- .../options/getCliOptions.test.ts | 32 +++++++ src/options/getCliOptions.ts | 95 ++++++++++++++++++- 2 files changed, 125 insertions(+), 2 deletions(-) diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index 88560e4b6..64e880254 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -122,6 +122,27 @@ describe('getCliOptions', () => { expect(() => getCliOptionsTest(['--disallowedChangeTypes', 'major'])).toThrow(); }); + it('suggests dashed form for camelCase boolean options', () => { + expect(() => getCliOptionsTest(['--gitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['--bumpDeps'])).toThrow('Did you mean --bump-deps or --no-bump-deps?'); + expect(() => getCliOptionsTest(['--keepChangeFiles'])).toThrow( + 'Did you mean --keep-change-files or --no-keep-change-files?' + ); + }); + + it('suggests dashed form for camelCase non-boolean options', () => { + expect(() => getCliOptionsTest(['--fromRef', 'main'])).toThrow('Did you mean --from-ref?'); + expect(() => getCliOptionsTest(['--dependentChangeType', 'patch'])).toThrow( + 'Did you mean --dependent-change-type?' + ); + }); + + it('suggests dashed --no- form for camelCase --noX options', () => { + expect(() => getCliOptionsTest(['--noFetch'])).toThrow('Did you mean --fetch or --no-fetch?'); + expect(() => getCliOptionsTest(['--noBump'])).toThrow('Did you mean --bump or --no-bump?'); + expect(() => getCliOptionsTest(['--noGitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + }); + it('parses short option aliases', () => { const options = getCliOptionsTest(['publish', '-t', 'test', '-r', 'http://whatever', '-y']); expect(options).toEqual({ ...defaults, command: 'publish', tag: 'test', registry: 'http://whatever', yes: true }); @@ -194,6 +215,17 @@ describe('getCliOptions', () => { expect(() => getCliOptionsTest(['--foo', 'bar', 'baz'])).toThrow(); }); + it('suggests --opt/--no-opt for near-match of a boolean flag', () => { + expect(() => getCliOptionsTest(['--fetc'])).toThrow('Did you mean --fetch or --no-fetch?'); + expect(() => getCliOptionsTest(['--git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['--bum'])).toThrow('Did you mean --bump or --no-bump?'); + }); + + it('suggests --opt/--no-opt for near-match of a --no-X flag', () => { + expect(() => getCliOptionsTest(['--no-git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['--no-bum'])).toThrow('Did you mean --bump or --no-bump?'); + }); + describe('config command', () => { it('parses config get with setting name', () => { const options = getCliOptionsTest(['config', 'get', 'branch']); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index d02874755..21c58724b 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -1,4 +1,4 @@ -import { Command, Option } from 'commander'; +import { Command, CommanderError, Option } from 'commander'; import type { ParsedOptions } from '../types/BeachballOptions'; import { getDefaultRemoteBranch, findProjectRoot } from 'workspace-tools'; import { env } from '../env'; @@ -109,6 +109,90 @@ function createProgram(): Command { return program; } +/** Convert a camelCase string to dashed form: e.g. `gitTags` => `git-tags` */ +function camelToDash(str: string): string { + return str.replace(/[A-Z]/g, letter => `-${letter.toLowerCase()}`); +} + +/** + * If the unknown option matches a boolean flag, suggest both `--opt` and `--no-opt`. + * If the unknown option is in camelCase, suggest the valid dashed form instead. + * Otherwise re-throw the original error unchanged. + */ +function enhanceUnknownOptionError(err: CommanderError, program: Command): never { + // Parse the unknown flag from commander's error message format: + // "error: unknown option '--foo'" or "error: unknown option '--foo'\n(Did you mean --bar?)" + const flagMatch = err.message.match(/^error: unknown option '(--[^']+)'/); + if (!flagMatch) { + throw err; + } + const unknownFlag = flagMatch[1]; + + // Collect info about known options from the program + const knownLongFlags = new Set(); + const negatableBooleans = new Set(); // long flags that have a --no-X counterpart + for (const opt of program.options) { + if (opt.long) { + knownLongFlags.add(opt.long); + // Track booleans that have a negation counterpart: if we see --no-X, mark --X as negatable + if (opt.negate && knownLongFlags.has(opt.long.replace(/^--no-/, '--'))) { + negatableBooleans.add(opt.long.replace(/^--no-/, '--')); + } + } + } + // Second pass: if we saw --X before --no-X, we need to check in reverse + for (const opt of program.options) { + if (opt.long && !opt.negate && knownLongFlags.has(`--no-${opt.long.slice(2)}`)) { + negatableBooleans.add(opt.long); + } + } + + // Check if the unknown flag contains uppercase and the dashed version is valid + const flagName = unknownFlag.replace(/^--/, ''); + if (/[A-Z]/.test(flagName)) { + const dashedFlag = `--${camelToDash(flagName)}`; + if (knownLongFlags.has(dashedFlag)) { + let suggestion: string; + if (negatableBooleans.has(dashedFlag)) { + suggestion = `(Did you mean ${dashedFlag} or --no-${dashedFlag.slice(2)}?)`; + } else if (dashedFlag.startsWith('--no-') && negatableBooleans.has(`--${dashedFlag.slice(5)}`)) { + // The dashed form is a --no-X flag and the positive form is negatable + suggestion = `(Did you mean --${dashedFlag.slice(5)} or ${dashedFlag}?)`; + } else { + suggestion = `(Did you mean ${dashedFlag}?)`; + } + throw new CommanderError(err.exitCode, err.code, `error: unknown option '${unknownFlag}'\n${suggestion}`); + } + } + + // Check if commander's existing suggestion matches a negatable boolean flag + const suggestMatch = err.message.match(/\(Did you mean (--[^ ?]+)\?\)/); + if (suggestMatch) { + const suggested = suggestMatch[1]; + if (negatableBooleans.has(suggested)) { + throw new CommanderError( + err.exitCode, + err.code, + `error: unknown option '${unknownFlag}'\n(Did you mean ${suggested} or --no-${suggested.slice(2)}?)` + ); + } + // If the suggestion itself is a --no-X and the positive form is negatable + if (suggested.startsWith('--no-')) { + const positiveFlag = `--${suggested.slice(5)}`; + if (negatableBooleans.has(positiveFlag)) { + throw new CommanderError( + err.exitCode, + err.code, + `error: unknown option '${unknownFlag}'\n(Did you mean ${positiveFlag} or ${suggested}?)` + ); + } + } + } + + // No enhancement possible, re-throw as-is + throw err; +} + /** * Gets CLI options. */ @@ -122,7 +206,14 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti : processOrArgv; const program = createProgram(); - program.parse(processInfo.argv); + try { + program.parse(processInfo.argv); + } catch (err) { + if (err instanceof CommanderError && err.code === 'commander.unknownOption') { + enhanceUnknownOptionError(err, program); + } + throw err; + } const commanderOpts = program.opts(); const positionalArgs = program.args; From 59743ddcc4d65f62e91a5c2c3507e517556f0415 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 06:24:35 +0000 Subject: [PATCH 4/7] fix: update argv in test files to include required command name The CLI parser now requires a command name in argv. Updated all test files that passed argv: [] to getParsedOptions to include the appropriate command name (bump, change, check, or publish) based on each test's context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/__e2e__/bump.test.ts | 2 +- src/__e2e__/getChangedPackages.test.ts | 2 +- src/__e2e__/validate.test.ts | 2 +- .../changefile/readChangeFiles.test.ts | 2 +- .../changelog/writeChangelog.test.ts | 2 +- .../options/getCliOptions.test.ts | 173 ++++++----- src/__tests__/bump/bumpInMemory.test.ts | 2 +- src/__tests__/bump/performBump.test.ts | 2 +- .../changefile/getQuestionsForPackage.test.ts | 6 +- src/__tests__/commands/configGet.test.ts | 35 +-- .../listPackageVersions.test.ts | 2 +- src/cli.ts | 41 +-- src/commands/configGet.ts | 6 +- src/help.ts | 92 ------ src/options/getCliOptions.ts | 292 ++++++++++++------ src/types/BeachballOptions.ts | 8 +- 16 files changed, 347 insertions(+), 322 deletions(-) diff --git a/src/__e2e__/bump.test.ts b/src/__e2e__/bump.test.ts index b5dcc2703..fecfd90e2 100644 --- a/src/__e2e__/bump.test.ts +++ b/src/__e2e__/bump.test.ts @@ -37,7 +37,7 @@ describe('bump command', () => { function getOptions(repoOptions?: Partial, cwd?: string) { const parsedOptions = getParsedOptions({ cwd: cwd || repo?.rootPath || '', - argv: [], + argv: ['node', 'beachball', 'bump'], testRepoOptions: { branch: defaultRemoteBranchName, fetch: false, ...repoOptions }, }); return { options: parsedOptions.options, parsedOptions }; diff --git a/src/__e2e__/getChangedPackages.test.ts b/src/__e2e__/getChangedPackages.test.ts index cd51a4112..b4a94caad 100644 --- a/src/__e2e__/getChangedPackages.test.ts +++ b/src/__e2e__/getChangedPackages.test.ts @@ -30,7 +30,7 @@ describe('getChangedPackages (basic)', () => { function getChangedPackagesWrapper(options?: Partial) { const parsedOptions = getParsedOptions({ cwd: reusedRepo.rootPath, - argv: [], + argv: ['node', 'beachball', 'change'], testRepoOptions: { fetch: false, branch: defaultRemoteBranchName, diff --git a/src/__e2e__/validate.test.ts b/src/__e2e__/validate.test.ts index c5be8103f..18286f8ae 100644 --- a/src/__e2e__/validate.test.ts +++ b/src/__e2e__/validate.test.ts @@ -15,7 +15,7 @@ describe('validate', () => { function validateWrapper(validateOptions?: ValidateOptions) { const parsedOptions = getParsedOptions({ cwd: repo!.rootPath, - argv: [], + argv: ['node', 'beachball', 'check'], testRepoOptions: { branch: defaultRemoteBranchName, }, diff --git a/src/__functional__/changefile/readChangeFiles.test.ts b/src/__functional__/changefile/readChangeFiles.test.ts index 5a8c6eda6..fe6ce06dd 100644 --- a/src/__functional__/changefile/readChangeFiles.test.ts +++ b/src/__functional__/changefile/readChangeFiles.test.ts @@ -30,7 +30,7 @@ describe('readChangeFiles', () => { expect(cwd).toBeTruthy(); const parsedOptions = getParsedOptions({ cwd: cwd!, - argv: [], + argv: ['node', 'beachball', 'change'], testRepoOptions: { branch: defaultRemoteBranchName, ...repoOptions }, }); const packageInfos = getPackageInfos(parsedOptions); diff --git a/src/__functional__/changelog/writeChangelog.test.ts b/src/__functional__/changelog/writeChangelog.test.ts index a04583a62..f6f72e2dd 100644 --- a/src/__functional__/changelog/writeChangelog.test.ts +++ b/src/__functional__/changelog/writeChangelog.test.ts @@ -37,7 +37,7 @@ describe('writeChangelog', () => { function getOptionsAndPackages(repoOptions?: Partial, cwd?: string) { const parsedOptions = getParsedOptions({ cwd: cwd || repo?.rootPath || '', - argv: [], + argv: ['node', 'beachball', 'bump'], testRepoOptions: { branch: defaultRemoteBranchName, ...repoOptions }, }); const packageInfos = getPackageInfos(parsedOptions); diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index 64e880254..d26081aa3 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -19,7 +19,6 @@ describe('getCliOptions', () => { // not allowed to access the surrounding context) const projectRoot = 'fake-root'; const mockFindProjectRoot = findProjectRoot as jest.MockedFunction; - const defaults = { command: 'change', path: projectRoot }; /** test wrapper for `getCliOptions` which adds common args */ function getCliOptionsTest(args: string[], cwd?: string) { @@ -38,69 +37,81 @@ describe('getCliOptions', () => { expect(findProjectRoot(process.cwd())).toEqual(projectRoot); }); - it('parses no args (adds path to result)', () => { - const options = getCliOptionsTest([]); - expect(options).toEqual(defaults); + it('requires a command', () => { + expect(() => getCliOptionsTest([])).toThrow(); }); it('parses command', () => { const options = getCliOptionsTest(['check']); - expect(options).toEqual({ ...defaults, command: 'check' }); + expect(options).toEqual({ command: 'check', path: projectRoot }); }); it('parses options', () => { // use a basic option of each value type (except arrays, tested later) - const options = getCliOptionsTest(['--type', 'patch', '--access=public', '--fetch', '--depth', '1']); - expect(options).toEqual({ ...defaults, type: 'patch', access: 'public', fetch: true, depth: 1 }); + const options = getCliOptionsTest(['change', '--type', 'patch', '--access=public', '--fetch', '--depth', '1']); + expect(options).toEqual({ + command: 'change', + path: projectRoot, + type: 'patch', + access: 'public', + fetch: true, + depth: 1, + }); }); it('parses command and options', () => { const options = getCliOptionsTest(['publish', '--tag', 'foo']); - expect(options).toEqual({ ...defaults, command: 'publish', tag: 'foo' }); + expect(options).toEqual({ command: 'publish', path: projectRoot, tag: 'foo' }); }); it('parses array options with multiple values', () => { - const options = getCliOptionsTest(['--scope', 'foo', 'bar']); - expect(options).toEqual({ ...defaults, scope: ['foo', 'bar'] }); + const options = getCliOptionsTest(['change', '--scope', 'foo', 'bar']); + expect(options).toEqual({ command: 'change', path: projectRoot, scope: ['foo', 'bar'] }); }); it('parses array option specified multiple times', () => { - const options = getCliOptionsTest(['--scope', 'foo', '--scope', 'bar']); - expect(options).toEqual({ ...defaults, scope: ['foo', 'bar'] }); + const options = getCliOptionsTest(['change', '--scope', 'foo', '--scope', 'bar']); + expect(options).toEqual({ command: 'change', path: projectRoot, scope: ['foo', 'bar'] }); }); // documenting that this is not currently supported (could change in the future if desired) it('does not parse values with commas as separate array entries', () => { - const options = getCliOptionsTest(['--scope', 'a,b', '--scope=c,d']); - expect(options).toEqual({ ...defaults, scope: ['a,b', 'c,d'] }); + const options = getCliOptionsTest(['change', '--scope', 'a,b', '--scope=c,d']); + expect(options).toEqual({ command: 'change', path: projectRoot, scope: ['a,b', 'c,d'] }); }); it('uses last value if non-array option is specified multiple times', () => { - const options = getCliOptionsTest(['--tag', 'foo', '--tag', 'baz']); - expect(options).toEqual({ ...defaults, tag: 'baz' }); + const options = getCliOptionsTest(['change', '--tag', 'foo', '--tag', 'baz']); + expect(options).toEqual({ command: 'change', path: projectRoot, tag: 'baz' }); }); it('parses negated boolean option', () => { - const options = getCliOptionsTest(['--no-fetch']); - expect(options).toEqual({ ...defaults, fetch: false }); + const options = getCliOptionsTest(['change', '--no-fetch']); + expect(options).toEqual({ command: 'change', path: projectRoot, fetch: false }); }); it('parses negated boolean options with --no-X syntax', () => { - const options = getCliOptionsTest(['--no-fetch', '--no-yes']); - expect(options).toEqual({ ...defaults, fetch: false, yes: false }); + const options = getCliOptionsTest(['change', '--no-fetch', '--no-yes']); + expect(options).toEqual({ command: 'change', path: projectRoot, fetch: false, yes: false }); }); it('throws on invalid numeric value', () => { - expect(() => getCliOptionsTest(['--depth', 'foo'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--depth', 'foo'])).toThrow(); }); it('converts hyphenated options to camel case', () => { - const options = getCliOptionsTest(['--git-tags', '--dependent-change-type', 'patch']); - expect(options).toEqual({ ...defaults, gitTags: true, dependentChangeType: 'patch' }); + const options = getCliOptionsTest(['change', '--git-tags', '--dependent-change-type', 'patch']); + expect(options).toEqual({ + command: 'change', + path: projectRoot, + gitTags: true, + dependentChangeType: 'patch', + }); }); it('requires hyphenated form for multi-word options', () => { const options = getCliOptionsTest([ + 'change', '--git-tags', '--dependent-change-type', 'patch', @@ -109,7 +120,8 @@ describe('getCliOptions', () => { 'minor', ]); expect(options).toEqual({ - ...defaults, + command: 'change', + path: projectRoot, gitTags: true, dependentChangeType: 'patch', disallowedChangeTypes: ['major', 'minor'], @@ -117,135 +129,158 @@ describe('getCliOptions', () => { }); it('rejects camelCase form of multi-word options', () => { - expect(() => getCliOptionsTest(['--gitTags'])).toThrow(); - expect(() => getCliOptionsTest(['--dependentChangeType', 'patch'])).toThrow(); - expect(() => getCliOptionsTest(['--disallowedChangeTypes', 'major'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--gitTags'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--dependentChangeType', 'patch'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--disallowedChangeTypes', 'major'])).toThrow(); }); it('suggests dashed form for camelCase boolean options', () => { - expect(() => getCliOptionsTest(['--gitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); - expect(() => getCliOptionsTest(['--bumpDeps'])).toThrow('Did you mean --bump-deps or --no-bump-deps?'); - expect(() => getCliOptionsTest(['--keepChangeFiles'])).toThrow( + expect(() => getCliOptionsTest(['change', '--gitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['change', '--bumpDeps'])).toThrow('Did you mean --bump-deps or --no-bump-deps?'); + expect(() => getCliOptionsTest(['change', '--keepChangeFiles'])).toThrow( 'Did you mean --keep-change-files or --no-keep-change-files?' ); }); it('suggests dashed form for camelCase non-boolean options', () => { - expect(() => getCliOptionsTest(['--fromRef', 'main'])).toThrow('Did you mean --from-ref?'); - expect(() => getCliOptionsTest(['--dependentChangeType', 'patch'])).toThrow( + expect(() => getCliOptionsTest(['change', '--fromRef', 'main'])).toThrow('Did you mean --from-ref?'); + expect(() => getCliOptionsTest(['change', '--dependentChangeType', 'patch'])).toThrow( 'Did you mean --dependent-change-type?' ); }); it('suggests dashed --no- form for camelCase --noX options', () => { - expect(() => getCliOptionsTest(['--noFetch'])).toThrow('Did you mean --fetch or --no-fetch?'); - expect(() => getCliOptionsTest(['--noBump'])).toThrow('Did you mean --bump or --no-bump?'); - expect(() => getCliOptionsTest(['--noGitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['change', '--noFetch'])).toThrow('Did you mean --fetch or --no-fetch?'); + expect(() => getCliOptionsTest(['change', '--noBump'])).toThrow('Did you mean --bump or --no-bump?'); + expect(() => getCliOptionsTest(['change', '--noGitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); }); it('parses short option aliases', () => { const options = getCliOptionsTest(['publish', '-t', 'test', '-r', 'http://whatever', '-y']); - expect(options).toEqual({ ...defaults, command: 'publish', tag: 'test', registry: 'http://whatever', yes: true }); + expect(options).toEqual({ + command: 'publish', + path: projectRoot, + tag: 'test', + registry: 'http://whatever', + yes: true, + }); }); it('parses long option aliases', () => { - const options = getCliOptionsTest(['--config', 'path/to/config.json', '--force', '--since', 'main']); - expect(options).toEqual({ ...defaults, configPath: 'path/to/config.json', forceVersions: true, fromRef: 'main' }); + const options = getCliOptionsTest(['change', '--config', 'path/to/config.json', '--force', '--since', 'main']); + expect(options).toEqual({ + command: 'change', + path: projectRoot, + configPath: 'path/to/config.json', + forceVersions: true, + fromRef: 'main', + }); }); it('for canary command, adds canary tag and ignores regular tag', () => { const options = getCliOptionsTest(['canary', '--tag', 'bar']); - expect(options).toEqual({ ...defaults, command: 'canary', tag: 'canary' }); + expect(options).toEqual({ command: 'canary', path: projectRoot, tag: 'canary' }); }); it('for canary command, uses canaryName as tag and ignores regular tag', () => { const options = getCliOptionsTest(['canary', '--canary-name', 'foo', '--tag', 'bar']); - expect(options).toEqual({ ...defaults, command: 'canary', canaryName: 'foo', tag: 'foo' }); + expect(options).toEqual({ command: 'canary', path: projectRoot, canaryName: 'foo', tag: 'foo' }); }); it('does not set tag to canaryName for non-canary command', () => { const options = getCliOptionsTest(['publish', '--canary-name', 'foo', '--tag', 'bar']); - expect(options).toEqual({ ...defaults, command: 'publish', canaryName: 'foo', tag: 'bar' }); + expect(options).toEqual({ command: 'publish', path: projectRoot, canaryName: 'foo', tag: 'bar' }); }); it('falls back to given cwd as path if findProjectRoot fails', () => { mockFindProjectRoot.mockImplementationOnce(() => { throw new Error('nope'); }); - const options = getCliOptionsTest([], 'somewhere'); - expect(options).toEqual({ ...defaults, path: 'somewhere' }); + const options = getCliOptionsTest(['change'], 'somewhere'); + expect(options).toEqual({ command: 'change', path: 'somewhere' }); }); it('uses provided branch if it contains a slash', () => { - const options = getCliOptionsTest(['--branch', 'someremote/foo']); - expect(options).toEqual({ ...defaults, branch: 'someremote/foo' }); + const options = getCliOptionsTest(['change', '--branch', 'someremote/foo']); + expect(options).toEqual({ command: 'change', path: projectRoot, branch: 'someremote/foo' }); // this is mocked at the top of the file // eslint-disable-next-line etc/no-deprecated expect(getDefaultRemoteBranch).not.toHaveBeenCalled(); }); it('adds default remote to branch without slash', () => { - const options = getCliOptionsTest(['--branch', 'foo']); - expect(options).toEqual({ ...defaults, branch: 'origin/foo' }); + const options = getCliOptionsTest(['change', '--branch', 'foo']); + expect(options).toEqual({ command: 'change', path: projectRoot, branch: 'origin/foo' }); // eslint-disable-next-line etc/no-deprecated expect(getDefaultRemoteBranch).toHaveBeenCalledWith({ branch: 'foo', verbose: undefined, cwd: projectRoot }); }); it('throws on unknown string options', () => { - expect(() => getCliOptionsTest(['--foo', 'bar'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--foo', 'bar'])).toThrow(); }); it('throws on unknown boolean flags', () => { - expect(() => getCliOptionsTest(['--foo'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--foo'])).toThrow(); }); it('throws on unknown negated boolean flags', () => { - expect(() => getCliOptionsTest(['--no-bar'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--no-bar'])).toThrow(); }); it('throws on unknown option with value', () => { - expect(() => getCliOptionsTest(['--foo', 'true'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--foo', 'true'])).toThrow(); }); it('throws on unknown option specified multiple times', () => { - expect(() => getCliOptionsTest(['--foo', 'bar', '--foo', 'baz'])).toThrow(); - }); - - it('throws on unknown option followed by positional', () => { - expect(() => getCliOptionsTest(['--foo', 'bar', 'baz'])).toThrow(); + expect(() => getCliOptionsTest(['change', '--foo', 'bar', '--foo', 'baz'])).toThrow(); }); it('suggests --opt/--no-opt for near-match of a boolean flag', () => { - expect(() => getCliOptionsTest(['--fetc'])).toThrow('Did you mean --fetch or --no-fetch?'); - expect(() => getCliOptionsTest(['--git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); - expect(() => getCliOptionsTest(['--bum'])).toThrow('Did you mean --bump or --no-bump?'); + expect(() => getCliOptionsTest(['change', '--fetc'])).toThrow('Did you mean --fetch or --no-fetch?'); + expect(() => getCliOptionsTest(['change', '--git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['change', '--bum'])).toThrow('Did you mean --bump or --no-bump?'); }); it('suggests --opt/--no-opt for near-match of a --no-X flag', () => { - expect(() => getCliOptionsTest(['--no-git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); - expect(() => getCliOptionsTest(['--no-bum'])).toThrow('Did you mean --bump or --no-bump?'); + expect(() => getCliOptionsTest(['change', '--no-git-tag'])).toThrow('Did you mean --git-tags or --no-git-tags?'); + expect(() => getCliOptionsTest(['change', '--no-bum'])).toThrow('Did you mean --bump or --no-bump?'); + }); + + it('throws on unknown command', () => { + expect(() => getCliOptionsTest(['unknown'])).toThrow(); + }); + + it('throws on extra positional arguments', () => { + expect(() => getCliOptionsTest(['check', 'extra'])).toThrow(); }); describe('config command', () => { it('parses config get with setting name', () => { const options = getCliOptionsTest(['config', 'get', 'branch']); - expect(options).toEqual({ ...defaults, command: 'config', _extraPositionalArgs: ['get', 'branch'] }); + expect(options).toEqual({ command: 'config get', path: projectRoot, configSettingName: 'branch' }); }); it('parses config get with setting name and options', () => { const options = getCliOptionsTest(['config', 'get', 'tag', '--package', 'my-pkg']); expect(options).toEqual({ - ...defaults, - command: 'config', - _extraPositionalArgs: ['get', 'tag'], + command: 'config get', + path: projectRoot, + configSettingName: 'tag', package: ['my-pkg'], }); }); - it('still throws for non-config command with extra positional args', () => { - expect(() => getCliOptionsTest(['check', 'extra'])).toThrow( - 'Only one positional argument (the command) is allowed' - ); + it('parses config list', () => { + const options = getCliOptionsTest(['config', 'list']); + expect(options).toEqual({ command: 'config list', path: projectRoot }); + }); + + it('throws on config get without setting name', () => { + expect(() => getCliOptionsTest(['config', 'get'])).toThrow(); + }); + + it('throws on config get with extra args', () => { + expect(() => getCliOptionsTest(['config', 'get', 'branch', 'extra'])).toThrow(); }); }); }); diff --git a/src/__tests__/bump/bumpInMemory.test.ts b/src/__tests__/bump/bumpInMemory.test.ts index d1349df37..4dc2fe039 100644 --- a/src/__tests__/bump/bumpInMemory.test.ts +++ b/src/__tests__/bump/bumpInMemory.test.ts @@ -20,7 +20,7 @@ describe('bumpInMemory', () => { }) { const { cliOptions, options } = getParsedOptions({ cwd, - argv: [], + argv: ['node', 'beachball', 'bump'], testRepoOptions: params.repoOptions, }); const originalPackageInfos = makePackageInfosByFolder({ diff --git a/src/__tests__/bump/performBump.test.ts b/src/__tests__/bump/performBump.test.ts index 2d133c978..6fe176234 100644 --- a/src/__tests__/bump/performBump.test.ts +++ b/src/__tests__/bump/performBump.test.ts @@ -71,7 +71,7 @@ describe('performBump', () => { }) { const opts = getParsedOptions({ cwd: fakeRoot, - argv: [], + argv: ['node', 'beachball', 'bump'], testRepoOptions: { branch: defaultRemoteBranchName, ...params.repoOptions }, }); diff --git a/src/__tests__/changefile/getQuestionsForPackage.test.ts b/src/__tests__/changefile/getQuestionsForPackage.test.ts index 684175d1a..c02aaf43f 100644 --- a/src/__tests__/changefile/getQuestionsForPackage.test.ts +++ b/src/__tests__/changefile/getQuestionsForPackage.test.ts @@ -32,7 +32,11 @@ describe('getQuestionsForPackage', () => { const packageInfos = makePackageInfos({ [pkg]: packageInfo }); // fill in default options - const { options } = getParsedOptions({ cwd: '', argv: [], testRepoOptions: repoOptions }); + const { options } = getParsedOptions({ + cwd: '', + argv: ['node', 'beachball', 'change'], + testRepoOptions: repoOptions, + }); return getQuestionsForPackage({ pkg, packageInfos, packageGroups, options, recentMessages }); } diff --git a/src/__tests__/commands/configGet.test.ts b/src/__tests__/commands/configGet.test.ts index 3d2f00a2b..99eb83187 100644 --- a/src/__tests__/commands/configGet.test.ts +++ b/src/__tests__/commands/configGet.test.ts @@ -11,12 +11,13 @@ import type { BeachballOptions, PackageOptions, VersionGroupOptions } from '../. describe('configGet', () => { const logs = initMockLogs(); - /** Wrapper that just provides custom args to `configGet` (for invalid argument cases) */ - function configGetArgs(args: string[]) { - configGet( - { ...getDefaultOptions(), _extraPositionalArgs: args }, - { originalPackageInfos: {}, scopedPackages: new Set(), packageGroups: {} } - ); + /** Wrapper that just provides custom name to `configGet` (for invalid argument cases) */ + function configGetWithName(name?: string) { + configGet({ ...getDefaultOptions(), command: 'config get', configSettingName: name } as BeachballOptions, { + originalPackageInfos: {}, + scopedPackages: new Set(), + packageGroups: {}, + }); } /** Get the given option (`name` will be formatted as args) with optional overrides */ @@ -32,7 +33,8 @@ describe('configGet', () => { const options: BeachballOptions = { ...getDefaultOptions(), - _extraPositionalArgs: ['get', name], + command: 'config get', + configSettingName: name, ...optionOverrides, }; const originalPackageInfos = makePackageInfos(packageInfos); @@ -45,28 +47,17 @@ describe('configGet', () => { } describe('argument validation', () => { - it('throws on missing subcommand', async () => { - await expectBeachballError(() => configGetArgs([]), 'Usage: beachball config get '); - }); - - it('throws on wrong subcommand', async () => { - await expectBeachballError(() => configGetArgs(['set', 'branch']), 'Usage: beachball config get '); - }); - - it('throws on too many args', async () => { - await expectBeachballError( - () => configGetArgs(['get', 'branch', 'extra']), - 'Usage: beachball config get ' - ); + it('throws on missing setting name', async () => { + await expectBeachballError(() => configGetWithName(), 'Usage: beachball config get '); }); it('throws on unknown config setting', async () => { - await expectBeachballError(() => configGetArgs(['get', 'nonExistent']), 'Unknown config setting: "nonExistent"'); + await expectBeachballError(() => configGetWithName('nonExistent'), 'Unknown config setting: "nonExistent"'); }); it('suggests similar config name on typo', async () => { await expectBeachballError( - () => configGetArgs(['get', 'branc']), + () => configGetWithName('branc'), 'Unknown config setting: "branc" - did you mean "branch"?' ); }); diff --git a/src/__tests__/packageManager/listPackageVersions.test.ts b/src/__tests__/packageManager/listPackageVersions.test.ts index 9383284d0..c707ab1ca 100644 --- a/src/__tests__/packageManager/listPackageVersions.test.ts +++ b/src/__tests__/packageManager/listPackageVersions.test.ts @@ -118,7 +118,7 @@ describe('list npm versions', () => { repoOptions?: Partial; }) { const parsedOptions = getParsedOptions({ - argv: ['node', 'beachball', ...(params.extraArgv || [])], + argv: ['node', 'beachball', 'publish', ...(params.extraArgv || [])], cwd: '', testRepoOptions: { registry, diff --git a/src/cli.ts b/src/cli.ts index c38720013..cdf2fcad9 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -8,7 +8,7 @@ import { init } from './commands/init'; import { publish } from './commands/publish'; import { sync } from './commands/sync'; -import { showVersion, showHelp } from './help'; +import { showVersion } from './help'; import { getPackageInfos } from './monorepo/getPackageInfos'; import { getParsedOptions } from './options/getOptions'; import { validate } from './validation/validate'; @@ -28,16 +28,6 @@ import { getPackageGroups } from './monorepo/getPackageGroups'; const parsedOptions = getParsedOptions({ cwd: process.cwd(), argv: process.argv }); const options = parsedOptions.options; - if (options.help) { - showHelp(); - return; - } - - if (options.version) { - showVersion(); - return; - } - // Run the commands switch (options.command) { case 'check': { @@ -96,17 +86,21 @@ import { getPackageGroups } from './monorepo/getPackageGroups'; break; } - case 'config': { + case 'config get': { const originalPackageInfos = getPackageInfos(parsedOptions); const scopedPackages = getScopedPackages(options, originalPackageInfos); const packageGroups = getPackageGroups(originalPackageInfos, options.path, options.groups); const configContext = { originalPackageInfos, scopedPackages, packageGroups }; - const subcommand = options._extraPositionalArgs?.[0]; - if (subcommand === 'list') { - configList(options, configContext); - } else { - configGet(options, configContext); - } + configGet(options, configContext); + break; + } + + case 'config list': { + const originalPackageInfos = getPackageInfos(parsedOptions); + const scopedPackages = getScopedPackages(options, originalPackageInfos); + const packageGroups = getPackageGroups(originalPackageInfos, options.path, options.groups); + const configContext = { originalPackageInfos, scopedPackages, packageGroups }; + configList(options, configContext); break; } @@ -114,6 +108,17 @@ import { getPackageGroups } from './monorepo/getPackageGroups'; throw new Error('Invalid command: ' + options.command); } })().catch(e => { + // Handle commander's --help and --version output (exit cleanly) + if (e && typeof e === 'object' && 'code' in e) { + const code = (e as { code: string }).code; + if (code === 'commander.helpDisplayed' || code === 'commander.version') { + if ((e as { message?: string }).message) { + console.log((e as { message: string }).message); + } + return; + } + } + if (e instanceof BeachballError && e.alreadyLogged) { // Error details were already printed -- just exit } else if (e instanceof BeachballError) { diff --git a/src/commands/configGet.ts b/src/commands/configGet.ts index dd319b2a4..509c632fa 100644 --- a/src/commands/configGet.ts +++ b/src/commands/configGet.ts @@ -82,14 +82,12 @@ const validConfigNames = new Set([ export function configGet(options: BeachballOptions, context: BasicCommandContext): void { const { originalPackageInfos: packageInfos, scopedPackages } = context; - const extraArgs = options._extraPositionalArgs || []; - if (extraArgs[0] !== 'get' || extraArgs.length !== 2) { + const name = options.configSettingName; + if (!name) { throw new BeachballError( 'Usage: beachball config get \n\nGets the value of the specified config setting.' ); } - - const name = extraArgs[1]; if (!validConfigNames.has(name)) { const suggestion = findSimilar(name, [...validConfigNames]); throw new BeachballError( diff --git a/src/help.ts b/src/help.ts index 3c6937034..fc31a7acf 100644 --- a/src/help.ts +++ b/src/help.ts @@ -6,95 +6,3 @@ export function showVersion(): void { const packageJson = readJson(path.resolve(__dirname, '../package.json')); console.log(`beachball v${packageJson.version} - the sunniest version bumping tool`); } - -export function showHelp(): void { - showVersion(); - - console.log(`Usage: - - beachball [command] [options] - -Examples: - - $ beachball - $ beachball check - $ beachball publish -r http://localhost:4873 -t beta -b beta - -Commands: - - change (default) - create change files in the change/ folder - check - checks whether a change file is needed for this branch - bump - bumps versions as well as generating changelogs - publish - bumps, publishes to npm registry (optionally does dist-tags), and - pushes changelogs back into the default branch - sync - synchronize published versions of packages from the registry with - local package.json versions - config get - get the value of a config setting (with any overrides) - config list - list all config settings (with any overrides) - -Options supported by all commands except 'config': - - --branch, -b - target branch from remote (default: git config init.defaultBranch) - --change-dir - name of the directory to store change files (default: change) - --config-path, -c - custom beachball config path (default: cosmiconfig standard paths) - --no-fetch - skip fetching from the remote before determining changes - --scope - only consider package paths matching this pattern - (can be specified multiple times; supports negations) - --since - consider changes or change files since this git ref (branch name, commit SHA) - --verbose - prints additional information to the console - -'change' options: - - --message, -m - description for all changed packages (instead of prompting) - --type - type of change: minor, patch, none, ... (instead of prompting) - --package, -p - force creating a change file for this package, regardless of diffs - (can be specified multiple times) - --all - generate change files for all packages - --dependent-change-type - use this change type for dependent packages (default patch) - --no-commit - stage change files only - -'check' options: - - --changehint - give your developers a customized hint message when they - forget to add a change file - --disallow-deleted-change-files - verifies that no change files were deleted between head and - target branch. - -'bump' options: - - --keep-change-files - don't delete the change files from disk after bumping - --prerelease-prefix - prerelease prefix for packages that will receive a prerelease bump - -'publish' options: - - Also supports all 'bump' options. - - --auth-type - npm auth type: 'authtoken' or 'password' - --message, -m - commit message (default: "applying package updates") - --no-bump - skip both bumping versions and pushing changes back to git remote - --no-git-tags - don't create git tags for each published package version - --no-publish - skip publishing to the npm registry - --no-push - skip committing changes and pushing them back to the git remote - --registry, -r - registry (default https://registry.npmjs.org) - --retries - number of retries for npm publishes (default: 3) - --tag, -t - dist-tag for npm publishes (default: "latest") - --token - npm token or password - --yes, -y - skip the confirmation prompts - -'sync' options: - - --registry, -r - registry (default https://registry.npmjs.org) - --tag, -t - sync to the specified npm dist-tag (default: 'latest') - --force - use the version from the registry even if it's older than local - -'config get ' options: - - --package, -p - get the effective value for specific package(s) - (can be specified multiple times) - -'config list' options: - - (no additional options) - -`); -} diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index 21c58724b..a6ee4c9fe 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -1,6 +1,9 @@ import { Command, CommanderError, Option } from 'commander'; +import path from 'path'; import type { ParsedOptions } from '../types/BeachballOptions'; +import type { PackageJson } from '../types/PackageInfo'; import { getDefaultRemoteBranch, findProjectRoot } from 'workspace-tools'; +import { readJson } from '../object/readJson'; import { env } from '../env'; export interface ProcessInfo { @@ -24,91 +27,96 @@ function parseNumber(value: string, optionName: string): number { return num; } -/** - * Creates and configures the commander program with all beachball options. - */ -function createProgram(): Command { - const program = new Command(); - program - .allowExcessArguments(true) - .exitOverride() // throw instead of calling process.exit - .configureOutput({ - // eslint-disable-next-line @typescript-eslint/no-empty-function - writeOut: () => {}, // suppress commander's built-in output - // eslint-disable-next-line @typescript-eslint/no-empty-function - writeErr: () => {}, - }); - - // Disable commander's built-in --help and --version so we can handle them ourselves - program.helpOption(false); - +/** Add all shared CLI options (for every command except `config get`) to a command. */ +function addSharedOptions(cmd: Command): Command { // -- Boolean options (with --no-X negation support) -- - program.option('--all'); - program.option('--bump'); - program.option('--no-bump'); - program.option('--bump-deps'); - program.option('--no-bump-deps'); - program.option('--commit'); - program.option('--no-commit'); - program.option('--disallow-deleted-change-files'); - program.option('--no-disallow-deleted-change-files'); - program.option('--fetch'); - program.option('--no-fetch'); - program.option('--force, --force-versions'); - program.option('--git-tags'); - program.option('--no-git-tags'); - program.option('-h, --help'); - program.option('--keep-change-files'); - program.option('--no-keep-change-files'); - program.option('--new'); - program.option('--no-new'); - program.option('--publish'); - program.option('--no-publish'); - program.option('--push'); - program.option('--no-push'); - program.option('--verbose'); - program.option('-v, --version'); - program.option('-y, --yes'); - program.option('--no-yes'); - - // -- Array options (variadic: accepts multiple values or repeated flags) -- - program.option('--disallowed-change-types '); - program.option('-p, --package '); - program.option('--scope '); + cmd.option('--all', 'consider all packages to have changed'); + cmd.option('--bump', 'bump versions during publish'); + cmd.option('--no-bump', 'skip bumping versions during publish'); + cmd.option('--bump-deps', 'bump dependent packages'); + cmd.option('--no-bump-deps', 'skip bumping dependent packages'); + cmd.option('--commit', 'commit change files after creating'); + cmd.option('--no-commit', 'stage change files only'); + cmd.option('--disallow-deleted-change-files', 'verify no change files were deleted'); + cmd.option('--no-disallow-deleted-change-files'); + cmd.option('--fetch', 'fetch from the remote before determining changes'); + cmd.option('--no-fetch', 'skip fetching from the remote'); + cmd.option('--force, --force-versions', 'use version from registry even if older than local'); + cmd.option('--git-tags', 'create git tags for published package versions'); + cmd.option('--no-git-tags', 'skip creating git tags'); + cmd.option('--keep-change-files', 'keep change files on disk after bumping'); + cmd.option('--no-keep-change-files'); + cmd.option('--new', 'publish newly added packages'); + cmd.option('--no-new'); + cmd.option('--publish', 'publish to the npm registry'); + cmd.option('--no-publish', 'skip publishing to the npm registry'); + cmd.option('--push', 'push to the remote git branch when publishing'); + cmd.option('--no-push', 'skip pushing to the git remote'); + cmd.option('--verbose', 'print additional information to the console'); + cmd.option('-y, --yes', 'skip confirmation prompts'); + cmd.option('--no-yes'); + + // -- Array options -- + cmd.option('--disallowed-change-types ', 'disallow these change types'); + cmd.option('-p, --package ', 'target specific packages'); + cmd.option('--scope ', 'only consider package paths matching these patterns'); // -- Number options -- - program.addOption(new Option('--concurrency ').argParser((v: string) => parseNumber(v, 'concurrency'))); - program.addOption(new Option('--depth ').argParser((v: string) => parseNumber(v, 'depth'))); - program.addOption( - new Option('--npm-read-concurrency ').argParser((v: string) => parseNumber(v, 'npmReadConcurrency')) + cmd.addOption( + new Option('--concurrency ', 'maximum concurrency for write operations').argParser((v: string) => + parseNumber(v, 'concurrency') + ) + ); + cmd.addOption( + new Option('--depth ', 'depth of git history for shallow clones').argParser((v: string) => + parseNumber(v, 'depth') + ) + ); + cmd.addOption( + new Option('--npm-read-concurrency ', 'maximum concurrency for npm registry reads').argParser((v: string) => + parseNumber(v, 'npmReadConcurrency') + ) + ); + cmd.addOption( + new Option('--git-timeout ', 'timeout for git push operations').argParser((v: string) => + parseNumber(v, 'gitTimeout') + ) + ); + cmd.addOption( + new Option('--retries ', 'number of retries for npm publish').argParser((v: string) => parseNumber(v, 'retries')) + ); + cmd.addOption( + new Option('--timeout ', 'timeout for npm operations').argParser((v: string) => parseNumber(v, 'timeout')) ); - program.addOption(new Option('--git-timeout ').argParser((v: string) => parseNumber(v, 'gitTimeout'))); - program.addOption(new Option('--retries ').argParser((v: string) => parseNumber(v, 'retries'))); - program.addOption(new Option('--timeout ').argParser((v: string) => parseNumber(v, 'timeout'))); // -- String options -- - program.option('--access '); - program.option('-a, --auth-type '); - program.option('-b, --branch '); - program.option('--canary-name '); - program.option('--changehint '); - program.option('--change-dir '); - program.option('-c, --config-path '); - // --config is a long alias for --config-path (commander only supports one long name per option) - program.addOption(new Option('--config ').hideHelp()); - program.option('--dependent-change-type '); - program.option('--since, --from-ref '); - program.option('-m, --message '); - program.option('--pack-to-path '); - program.option('--prerelease-prefix '); - program.option('-r, --registry '); - program.option('-t, --tag '); - program.option('-n, --token '); - program.option('--type '); - - return program; + cmd.option('--access ', "access level for npm publish: 'public' or 'restricted'"); + cmd.option('-a, --auth-type ', "npm auth type: 'authtoken' or 'password'"); + cmd.option('-b, --branch ', 'target branch from remote'); + cmd.option('--canary-name ', 'canary prerelease name'); + cmd.option('--changehint ', 'custom hint message when change files are needed'); + cmd.option('--change-dir ', 'directory to store change files'); + cmd.option('-c, --config-path ', 'custom beachball config path'); + cmd.addOption(new Option('--config ').hideHelp()); // hidden alias for --config-path + cmd.option('--dependent-change-type ', 'change type for dependent packages'); + cmd.option('--since, --from-ref ', 'consider changes since this git ref'); + cmd.option('-m, --message ', 'change description or commit message'); + cmd.option('--pack-to-path ', 'pack packages to this path instead of publishing'); + cmd.option('--prerelease-prefix ', 'prerelease prefix for prerelease bumps'); + cmd.option('-r, --registry ', 'target npm registry'); + cmd.option('-t, --tag ', 'dist-tag for npm publishes'); + cmd.option('-n, --token ', 'npm token or password'); + cmd.option('--type ', 'type of change: major, minor, patch, none, ...'); + + return cmd; } +/** + * Reference command with all shared options, used for error enhancement only. + * (Separate from the real commands to avoid circular references.) + */ +const referenceCommand = addSharedOptions(new Command()); + /** Convert a camelCase string to dashed form: e.g. `gitTags` => `git-tags` */ function camelToDash(str: string): string { return str.replace(/[A-Z]/g, letter => `-${letter.toLowerCase()}`); @@ -119,7 +127,7 @@ function camelToDash(str: string): string { * If the unknown option is in camelCase, suggest the valid dashed form instead. * Otherwise re-throw the original error unchanged. */ -function enhanceUnknownOptionError(err: CommanderError, program: Command): never { +function enhanceUnknownOptionError(err: CommanderError): never { // Parse the unknown flag from commander's error message format: // "error: unknown option '--foo'" or "error: unknown option '--foo'\n(Did you mean --bar?)" const flagMatch = err.message.match(/^error: unknown option '(--[^']+)'/); @@ -128,20 +136,19 @@ function enhanceUnknownOptionError(err: CommanderError, program: Command): never } const unknownFlag = flagMatch[1]; - // Collect info about known options from the program + // Collect info about known options from the reference command const knownLongFlags = new Set(); const negatableBooleans = new Set(); // long flags that have a --no-X counterpart - for (const opt of program.options) { + for (const opt of referenceCommand.options) { if (opt.long) { knownLongFlags.add(opt.long); - // Track booleans that have a negation counterpart: if we see --no-X, mark --X as negatable if (opt.negate && knownLongFlags.has(opt.long.replace(/^--no-/, '--'))) { negatableBooleans.add(opt.long.replace(/^--no-/, '--')); } } } // Second pass: if we saw --X before --no-X, we need to check in reverse - for (const opt of program.options) { + for (const opt of referenceCommand.options) { if (opt.long && !opt.negate && knownLongFlags.has(`--no-${opt.long.slice(2)}`)) { negatableBooleans.add(opt.long); } @@ -156,7 +163,6 @@ function enhanceUnknownOptionError(err: CommanderError, program: Command): never if (negatableBooleans.has(dashedFlag)) { suggestion = `(Did you mean ${dashedFlag} or --no-${dashedFlag.slice(2)}?)`; } else if (dashedFlag.startsWith('--no-') && negatableBooleans.has(`--${dashedFlag.slice(5)}`)) { - // The dashed form is a --no-X flag and the positive form is negatable suggestion = `(Did you mean --${dashedFlag.slice(5)} or ${dashedFlag}?)`; } else { suggestion = `(Did you mean ${dashedFlag}?)`; @@ -176,7 +182,6 @@ function enhanceUnknownOptionError(err: CommanderError, program: Command): never `error: unknown option '${unknownFlag}'\n(Did you mean ${suggested} or --no-${suggested.slice(2)}?)` ); } - // If the suggestion itself is a --no-X and the positive form is negatable if (suggested.startsWith('--no-')) { const positiveFlag = `--${suggested.slice(5)}`; if (negatableBooleans.has(positiveFlag)) { @@ -193,6 +198,84 @@ function enhanceUnknownOptionError(err: CommanderError, program: Command): never throw err; } +interface CommandMatch { + command: string; + opts: Record; + configSettingName?: string; +} + +/** Suppress stderr but capture stdout (for help/version output). */ +function makeOutputConfig(captured: { output: string }) { + return { + writeOut: (str: string) => { + captured.output += str; + }, + // eslint-disable-next-line @typescript-eslint/no-empty-function + writeErr: () => {}, + }; +} + +/** + * Creates and configures the commander program with all beachball commands and options. + */ +function createProgram(): { + program: Command; + getMatch: () => CommandMatch | undefined; + captured: { output: string }; +} { + let match: CommandMatch | undefined; + const captured = { output: '' }; + const outputConfig = makeOutputConfig(captured); + + const packageJson = readJson(path.resolve(__dirname, '../../package.json')); + + const program = new Command('beachball') + .description('the sunniest version bumping tool') + .version(`beachball v${packageJson.version} - the sunniest version bumping tool`, '-v, --version') + .exitOverride() + .configureOutput(outputConfig); + + // Define standard commands — each gets all shared options + const commandDefs = [ + { name: 'change', desc: 'create change files in the change/ folder' }, + { name: 'check', desc: 'check whether a change file is needed for this branch' }, + { name: 'bump', desc: 'bump versions and generate changelogs' }, + { name: 'publish', desc: 'bump, publish to npm registry, and push changelogs' }, + { name: 'canary', desc: 'publish canary prerelease versions' }, + { name: 'init', desc: 'initialize beachball config' }, + { name: 'sync', desc: 'synchronize published versions from the registry' }, + ]; + + for (const { name, desc } of commandDefs) { + const cmd = program.command(name).description(desc); + cmd.exitOverride().configureOutput(outputConfig).allowExcessArguments(false); + addSharedOptions(cmd); + cmd.action(() => { + match = { command: name, opts: cmd.opts() }; + }); + } + + // Config command with subcommands + const config = program.command('config').description('view configuration'); + config.exitOverride().configureOutput(outputConfig); + + const configGet = config.command('get').description('get the value of a config setting'); + configGet.argument('', 'config setting name'); + configGet.option('-p, --package ', 'get effective value for specific package(s)'); + configGet.exitOverride().configureOutput(outputConfig).allowExcessArguments(false); + configGet.action((name: string) => { + match = { command: 'config get', opts: configGet.opts(), configSettingName: name }; + }); + + const configList = config.command('list').description('list all config settings'); + configList.exitOverride().configureOutput(outputConfig).allowExcessArguments(false); + configList.action(() => { + match = { command: 'config list', opts: configList.opts() }; + }); + + return { program, getMatch: () => match, captured }; +} + /** * Gets CLI options. */ @@ -205,18 +288,29 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti { argv: processOrArgv, cwd: env.isJest ? '' : process.cwd() } : processOrArgv; - const program = createProgram(); + const { program, getMatch, captured } = createProgram(); + try { program.parse(processInfo.argv); } catch (err) { - if (err instanceof CommanderError && err.code === 'commander.unknownOption') { - enhanceUnknownOptionError(err, program); + if (err instanceof CommanderError) { + if (err.code === 'commander.helpDisplayed' || err.code === 'commander.version') { + // Re-throw with captured output so the caller can print it + throw new CommanderError(err.exitCode, err.code, captured.output || err.message); + } + if (err.code === 'commander.unknownOption') { + enhanceUnknownOptionError(err); + } } throw err; } - const commanderOpts = program.opts(); - const positionalArgs = program.args; + const match = getMatch(); + if (!match) { + // No command was matched — show help + program.outputHelp(); + throw new CommanderError(0, 'commander.helpDisplayed', captured.output); + } let cwd = processInfo.cwd; try { @@ -229,20 +323,19 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti // use the provided cwd } - // Determine command from positional args - const command = positionalArgs.length ? String(positionalArgs[0]) : 'change'; - - if (positionalArgs.length > 1 && command !== 'config') { - throw new Error(`Only one positional argument (the command) is allowed. Received: ${positionalArgs.join(' ')}`); - } - // Build the cliOptions object from commander's parsed options. // Only include options that were explicitly set on the command line (not undefined defaults). const cliOptions: ParsedOptions['cliOptions'] = { - command, + command: match.command, path: cwd, }; + if (match.configSettingName !== undefined) { + cliOptions.configSettingName = match.configSettingName; + } + + const commanderOpts = { ...match.opts }; + // Handle --config as alias for --config-path if (commanderOpts.config !== undefined) { if (commanderOpts.configPath !== undefined) { @@ -279,10 +372,5 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti cliOptions.tag = cliOptions.canaryName || 'canary'; } - // Save extra positional args for commands that support subcommands (e.g. 'config get ') - if (positionalArgs.length > 1) { - cliOptions._extraPositionalArgs = positionalArgs.slice(1).map(String); - } - return cliOptions; } diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts index 48e9a578b..b77aa63fc 100644 --- a/src/types/BeachballOptions.ts +++ b/src/types/BeachballOptions.ts @@ -68,12 +68,8 @@ export interface CliOptions version?: boolean; yes: boolean; - /** - * Extra positional arguments after the command (for subcommands like `config get `). - * This is a workaround for `yargs-parser`'s lack of positional argument support. - * @internal - */ - _extraPositionalArgs?: string[]; + /** The config setting name for `config get `. Set by the CLI parser. */ + configSettingName?: string; // ONLY add new options here if they only make sense on the command line! // Most options should be defined in RepoOptions and added to the Pick<...> above. From 617f8870c0316abe93fb11c095158cd777d699c0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 06:41:07 +0000 Subject: [PATCH 5/7] Restructure CLI to use commander subcommands with proper help text - Use .command() for all known commands (change, check, bump, publish, canary, init, sync) - Add config get and config list as proper subcommands with positional arg - Add option descriptions for commander-generated help text - Remove default command (subcommand is now required) - Replace _extraPositionalArgs with configSettingName - Remove showHelp() in favor of commander's built-in help - Remove CliOptions.help and CliOptions.version (commander handles these) - Handle commander help/version errors in cli.ts catch block - Use standard program.version() format - Update all tests for new subcommand structure Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/637df71a-fa66-490b-98ee-ab96f3469c64 --- src/__functional__/options/getOptions.test.ts | 10 +++++----- src/__tests__/commands/configList.test.ts | 1 - src/options/getCliOptions.ts | 15 ++++++++++----- src/options/getDefaultOptions.ts | 1 - src/types/BeachballOptions.ts | 2 -- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/__functional__/options/getOptions.test.ts b/src/__functional__/options/getOptions.test.ts index 19c536eda..4a5bef03a 100644 --- a/src/__functional__/options/getOptions.test.ts +++ b/src/__functional__/options/getOptions.test.ts @@ -14,7 +14,7 @@ describe('getOptions (deprecated)', () => { // Don't reuse a repo in these tests! If multiple tests load beachball.config.js from the same path, // it will use the version from the require cache, which will have outdated contents. - const baseArgv = () => ['node.exe', 'bin.js']; + const baseArgv = () => ['node.exe', 'bin.js', 'change']; const inDirectory = (directory: string, cb: () => T): T => { const originalDirectory = process.cwd(); @@ -109,7 +109,7 @@ describe('getParsedOptions', () => { // it will use the version from the require cache, which will have outdated contents. // Return a new object each time since getRepoOptions caches the result based on object identity. - const baseArgv = () => ['node', 'beachball', 'stuff']; + const baseArgv = () => ['node', 'beachball', 'change']; beforeAll(() => { repositoryFactory = new RepositoryFactory('single'); @@ -130,7 +130,7 @@ describe('getParsedOptions', () => { const parsedOptions = getParsedOptions({ argv: baseArgv(), cwd: repo.rootPath }); expect(parsedOptions).toEqual({ options: expect.objectContaining({ branch: 'origin/foo' }), - cliOptions: { path: repo.rootPath, command: 'stuff' }, + cliOptions: { path: repo.rootPath, command: 'change' }, }); }); @@ -141,7 +141,7 @@ describe('getParsedOptions', () => { const parsedOptions = getParsedOptions({ argv: baseArgv(), cwd: repo.rootPath }); expect(parsedOptions).toEqual({ options: expect.objectContaining({ branch: 'origin/foo' }), - cliOptions: { path: repo.rootPath, command: 'stuff' }, + cliOptions: { path: repo.rootPath, command: 'change' }, }); }); @@ -179,7 +179,7 @@ describe('getParsedOptions', () => { }); expect(parsedOptions).toEqual({ options: expect.objectContaining({ branch: 'origin/bar' }), - cliOptions: { path: repo.rootPath, command: 'stuff', branch: 'origin/bar' }, + cliOptions: { path: repo.rootPath, command: 'change', branch: 'origin/bar' }, }); }); diff --git a/src/__tests__/commands/configList.test.ts b/src/__tests__/commands/configList.test.ts index cedf7f183..de6d2bbff 100644 --- a/src/__tests__/commands/configList.test.ts +++ b/src/__tests__/commands/configList.test.ts @@ -83,7 +83,6 @@ describe('configList', () => { tag: "" timeout: undefined type: null - version: false yes: true" `); }); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index a6ee4c9fe..dfa8324d3 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -117,6 +117,14 @@ function addSharedOptions(cmd: Command): Command { */ const referenceCommand = addSharedOptions(new Command()); +/** Version string, read once at module load (before tests can mock fs). */ +const beachballVersion = (() => { + try { + return readJson(path.resolve(__dirname, '../../package.json')).version || 'unknown'; + } catch { + return 'unknown'; + } +})(); /** Convert a camelCase string to dashed form: e.g. `gitTags` => `git-tags` */ function camelToDash(str: string): string { return str.replace(/[A-Z]/g, letter => `-${letter.toLowerCase()}`); @@ -227,11 +235,9 @@ function createProgram(): { const captured = { output: '' }; const outputConfig = makeOutputConfig(captured); - const packageJson = readJson(path.resolve(__dirname, '../../package.json')); - const program = new Command('beachball') .description('the sunniest version bumping tool') - .version(`beachball v${packageJson.version} - the sunniest version bumping tool`, '-v, --version') + .version(beachballVersion, '-v, --version') .exitOverride() .configureOutput(outputConfig); @@ -341,7 +347,6 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti if (commanderOpts.configPath !== undefined) { throw new Error('Cannot specify both --config and --config-path'); } - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment commanderOpts.configPath = commanderOpts.config; } delete commanderOpts.config; @@ -350,7 +355,7 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti // Commander already converts hyphenated option names to camelCase in its opts() output. for (const [key, value] of Object.entries(commanderOpts)) { if (value !== undefined) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access (cliOptions as any)[key] = value; } } diff --git a/src/options/getDefaultOptions.ts b/src/options/getDefaultOptions.ts index f8e6090bd..0bc4d13e8 100644 --- a/src/options/getDefaultOptions.ts +++ b/src/options/getDefaultOptions.ts @@ -37,7 +37,6 @@ export function getDefaultOptions(): BeachballOptions { tag: '', timeout: undefined, type: null, - version: false, yes: env.isCI, }; } diff --git a/src/types/BeachballOptions.ts b/src/types/BeachballOptions.ts index b77aa63fc..65673922b 100644 --- a/src/types/BeachballOptions.ts +++ b/src/types/BeachballOptions.ts @@ -59,13 +59,11 @@ export interface CliOptions * For sync: use the version from the registry even if it's older than local. */ forceVersions?: boolean; - help?: boolean; /** Force change files for these packages */ package?: string | string[]; token?: string; type?: ChangeType | null; verbose?: boolean; - version?: boolean; yes: boolean; /** The config setting name for `config get `. Set by the CLI parser. */ From 44c69104bef4b9eed68797913b41f2bc82ce7ed8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 06:49:19 +0000 Subject: [PATCH 6/7] Address review: combine unknown option tests, make --config primary with --config-path alias Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/13e97a54-84b3-4b4c-adf8-9f0ff9ae878d --- src/__functional__/options/getCliOptions.test.ts | 14 +------------- src/options/getCliOptions.ts | 6 +++--- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index d26081aa3..dfdd8e3c9 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -215,23 +215,11 @@ describe('getCliOptions', () => { expect(getDefaultRemoteBranch).toHaveBeenCalledWith({ branch: 'foo', verbose: undefined, cwd: projectRoot }); }); - it('throws on unknown string options', () => { + it('throws on unknown options', () => { expect(() => getCliOptionsTest(['change', '--foo', 'bar'])).toThrow(); - }); - - it('throws on unknown boolean flags', () => { expect(() => getCliOptionsTest(['change', '--foo'])).toThrow(); - }); - - it('throws on unknown negated boolean flags', () => { expect(() => getCliOptionsTest(['change', '--no-bar'])).toThrow(); - }); - - it('throws on unknown option with value', () => { expect(() => getCliOptionsTest(['change', '--foo', 'true'])).toThrow(); - }); - - it('throws on unknown option specified multiple times', () => { expect(() => getCliOptionsTest(['change', '--foo', 'bar', '--foo', 'baz'])).toThrow(); }); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index dfa8324d3..d8092d5ab 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -96,8 +96,8 @@ function addSharedOptions(cmd: Command): Command { cmd.option('--canary-name ', 'canary prerelease name'); cmd.option('--changehint ', 'custom hint message when change files are needed'); cmd.option('--change-dir ', 'directory to store change files'); - cmd.option('-c, --config-path ', 'custom beachball config path'); - cmd.addOption(new Option('--config ').hideHelp()); // hidden alias for --config-path + cmd.option('-c, --config ', 'custom beachball config path'); + cmd.addOption(new Option('--config-path ').hideHelp()); // hidden alias for --config cmd.option('--dependent-change-type ', 'change type for dependent packages'); cmd.option('--since, --from-ref ', 'consider changes since this git ref'); cmd.option('-m, --message ', 'change description or commit message'); @@ -342,7 +342,7 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti const commanderOpts = { ...match.opts }; - // Handle --config as alias for --config-path + // Handle --config-path as alias for --config (both map to configPath) if (commanderOpts.config !== undefined) { if (commanderOpts.configPath !== undefined) { throw new Error('Cannot specify both --config and --config-path'); From 52259fe33958f3797dd6bed2f3d36bf0567e59e8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 28 Mar 2026 06:54:47 +0000 Subject: [PATCH 7/7] Address review: remove redundant comment lines and test, add TODO for alias upgrade, update docs Agent-Logs-Url: https://github.com/microsoft/beachball/sessions/1f3e4a87-1bdd-40c9-a3e1-53277fb750ba --- docs/cli/options.md | 18 +++++++++--------- .../options/getCliOptions.test.ts | 8 -------- src/options/getCliOptions.ts | 4 +++- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/docs/cli/options.md b/docs/cli/options.md index a64bf04eb..5989de05d 100644 --- a/docs/cli/options.md +++ b/docs/cli/options.md @@ -15,15 +15,15 @@ For the latest full list of supported options, see `CliOptions` [in this file](h The options below apply to most CLI commands. -| Option | Alias | Default | Description | -| --------------- | ----- | ------------------------- | -------------------------------------------------------------------------------------------- | -| `--branch, -b` | `-b` | | target branch; see [config docs][1] for details | -| `--config-path` | `-c` | [cosmiconfig][2] defaults | custom beachball config path | -| `--no-fetch` | | | skip fetching from the remote | -| `--change-dir` | | `'change'` | name of the directory to store change files | -| `--scope` | | | only consider matching package paths (can be specified multiple times); see [config docs][3] | -| `--since` | | | only consider changes or change files since this git ref (branch name, commit SHA) | -| `--verbose` | | | prints additional information to the console | +| Option | Alias | Default | Description | +| -------------- | ----- | ------------------------- | -------------------------------------------------------------------------------------------- | +| `--branch, -b` | `-b` | | target branch; see [config docs][1] for details | +| `--config` | `-c` | [cosmiconfig][2] defaults | custom beachball config path (alias: `--config-path`) | +| `--no-fetch` | | | skip fetching from the remote | +| `--change-dir` | | `'change'` | name of the directory to store change files | +| `--scope` | | | only consider matching package paths (can be specified multiple times); see [config docs][3] | +| `--since` | | | only consider changes or change files since this git ref (branch name, commit SHA) | +| `--verbose` | | | prints additional information to the console | [1]: ../overview/configuration#determining-the-target-branch-and-remote [2]: https://www.npmjs.com/package/cosmiconfig diff --git a/src/__functional__/options/getCliOptions.test.ts b/src/__functional__/options/getCliOptions.test.ts index dfdd8e3c9..ecdbaa62d 100644 --- a/src/__functional__/options/getCliOptions.test.ts +++ b/src/__functional__/options/getCliOptions.test.ts @@ -11,8 +11,6 @@ jest.mock('workspace-tools', () => { // // These tests cover a mix of built-in parser behavior, provided options, and custom overrides. -// The parser is commander, and these tests document the expected behavior from the beachball -// "end user" perspective. // describe('getCliOptions', () => { // This is the same mocked value as above (can't be shared in a const because jest.mock() is @@ -128,12 +126,6 @@ describe('getCliOptions', () => { }); }); - it('rejects camelCase form of multi-word options', () => { - expect(() => getCliOptionsTest(['change', '--gitTags'])).toThrow(); - expect(() => getCliOptionsTest(['change', '--dependentChangeType', 'patch'])).toThrow(); - expect(() => getCliOptionsTest(['change', '--disallowedChangeTypes', 'major'])).toThrow(); - }); - it('suggests dashed form for camelCase boolean options', () => { expect(() => getCliOptionsTest(['change', '--gitTags'])).toThrow('Did you mean --git-tags or --no-git-tags?'); expect(() => getCliOptionsTest(['change', '--bumpDeps'])).toThrow('Did you mean --bump-deps or --no-bump-deps?'); diff --git a/src/options/getCliOptions.ts b/src/options/getCliOptions.ts index d8092d5ab..993f4a303 100644 --- a/src/options/getCliOptions.ts +++ b/src/options/getCliOptions.ts @@ -342,7 +342,9 @@ export function getCliOptions(processOrArgv: ProcessInfo | string[]): ParsedOpti const commanderOpts = { ...match.opts }; - // Handle --config-path as alias for --config (both map to configPath) + // Handle --config-path as alias for --config (both map to configPath). + // TODO: Replace this manual alias handling with `new Option(...).alias('--config-path')` + // when upgrading to a commander version that supports .alias() on Option. if (commanderOpts.config !== undefined) { if (commanderOpts.configPath !== undefined) { throw new Error('Cannot specify both --config and --config-path');