feat(commands): add ctx.fail and warn on colliding option aliases

`ctx.fail(message)` is the failure verb on the command context, in both
`run` and `canExecute`. It maps to the errors service's `failWithHelp`,
so a command failure carries the usage suggestion, and returns `never` so
it can end a branch without a return. The message is validated like the
define-time errors are, naming the command. Throwing keeps working
unchanged — fail() is sugar over it, not a replacement. Commands get no
`skip()`: warn-and-continue has no meaning inside run().

The CLI-wide option collision warning now covers aliases on both sides,
so an `alias: "p"` that shadows `--path`'s shorthand is reported the same
way a `verbose` option shadowing `--verbose` is, naming both sides.

Author: edusperoni

defining-commands.md

@@ -155,7 +155,11 @@ const buildOptions = {
 the CLI itself. Declaring one of those names in a command's schema makes the
 command's declaration win for the duration of that command, which means the
 same flag means different things depending on which command is running. The CLI
-warns at registration naming the collision; pick another name.
+warns at registration naming both sides of the collision; pick another name.
+
+Aliases count too, in both directions: an `alias: "p"` collides with `--path`'s
+shorthand just as `output: stringOption()` would collide with a CLI-wide
+`--output`.
 
 ### How validation behaves
 
@@ -211,10 +215,10 @@ arguments even when it supplies a `canExecute`, and a `canExecute` that only
 inspects options cannot accidentally widen what the command accepts.
 
 `canExecute` receives a context of the same shape as `run`'s — the same
-`args` and the same declared options, built freshly for the call — and returns
-a boolean (or a promise of one). Returning `false` aborts the command and
-prints a help suggestion; throwing surfaces your own error message, which is
-usually the friendlier choice.
+`args`, the same declared options and the same `fail` — built freshly for the
+call, and returns a boolean (or a promise of one). Returning `false` aborts the
+command and prints a bare help suggestion; `ctx.fail(message)` aborts it with
+your own message, which is usually the friendlier choice.
 
 `canExecute` runs inside a dependency-injection context, on the same terms as
 `run`: `inject()` is valid up to the first `await`.
@@ -228,11 +232,38 @@ The run context
   (including any subcommand segments) has been consumed.
 - `ctx.options` — the current value of each declared option, read at the moment
   the command executes.
+- `ctx.fail(message)` — fails the command with `message` and a usage help
+  suggestion.
 
 `run` may be synchronous or `async`; the CLI awaits the result and treats a
-rejection as a command failure. Throwing is how a definition fails a command;
-`$errors.failWithHelp` from the injected `errors` service adds the help
-suggestion.
+rejection as a command failure.
+
+### Failing a command
+
+`ctx.fail(message)` is the idiomatic way to stop a command:
+
+```ts
+defineCommand({
+	name: "widget|add",
+	arguments: "any",
+	options: { output: stringOption() },
+	async run(ctx) {
+		if (!ctx.options.output) {
+			ctx.fail("--output is required.");
+		}
+
+		/* ... */
+	},
+});
+```
+
+It is available on the `canExecute` context as well, and it returns `never`, so
+it can end a branch without a `return`. The message must be a non-empty string.
+
+Throwing is equivalent and keeps working — `ctx.fail` is sugar over the
+`errors` service's `failWithHelp`, which is what adds the "Run `ns widget add
+--help`" line. Throw when you already have an `Error` to propagate; call
+`ctx.fail` when you are writing the message.
 
 `run` starts inside a dependency-injection context, so `inject()` works
 directly:

lib/common/define-command.ts

@@ -70,6 +70,8 @@ export interface CommandContext<TSchema extends CommandOptionsSchema = {}> {
 	args: string[];
 	/** Current value of every option declared in the schema, and nothing else. */
 	options: CommandOptionValues<TSchema>;
+	/** Fails the command with `message` and the usage help suggestion. */
+	fail(message: string): never;
 }
 
 export interface CommandDefinition<TSchema extends CommandOptionsSchema = {}> {

lib/common/services/command-definition-adapter.ts

@@ -1,7 +1,7 @@
 import { OptionType } from "../enums";
 import { injector } from "../yok";
 import { runInInjectionContext } from "../di/inject";
-import { IDictionary, IDashedOption } from "../declarations";
+import { IDictionary, IDashedOption, IErrors } from "../declarations";
 import { IInjector } from "../definitions/yok";
 import { ICommand } from "../definitions/commands";
 import { CommandRegistry } from "../contracts/command-registry";
@@ -51,23 +51,51 @@ const compileOptions = (
 	return dashedOptions;
 };
 
+const aliasList = (alias: string | string[]): string[] =>
+	alias === undefined ? [] : Array.isArray(alias) ? alias : [alias];
+
 /**
  * A command option that shadows a CLI-wide one wins the re-parse for this
- * command only, so the two spellings mean different things depending on what
- * the user typed first. Warned rather than rejected while the policy is open.
+ * command only, so the same spelling means different things depending on which
+ * command is running. Warned rather than rejected while the policy is open.
  */
 const warnOnCliOptionCollisions = (
 	targetInjector: IInjector,
 	definition: CommandDefinition<any>,
-	optionNames: string[],
+	schema: CommandOptionsSchema,
 	optionsService: any,
 ): void => {
 	const cliOptions = optionsService && optionsService.options;
 	if (!cliOptions) {
 		return;
 	}
 
-	const collisions = optionNames.filter((optionName) => cliOptions[optionName]);
+	// Every spelling the CLI already answers to, mapped to the option owning it.
+	const cliSpellings: IDictionary<string> = {};
+	for (const cliName of Object.keys(cliOptions)) {
+		cliSpellings[cliName] = cliName;
+		for (const alias of aliasList(cliOptions[cliName].alias)) {
+			cliSpellings[alias] = cliName;
+		}
+	}
+
+	const collisions: string[] = [];
+	for (const optionName of Object.keys(schema)) {
+		if (cliSpellings[optionName]) {
+			collisions.push(
+				`'--${optionName}' with the CLI option '--${cliSpellings[optionName]}'`,
+			);
+		}
+
+		for (const alias of aliasList(schema[optionName].alias)) {
+			if (cliSpellings[alias]) {
+				collisions.push(
+					`alias '-${alias}' of '--${optionName}' with the CLI option '--${cliSpellings[alias]}'`,
+				);
+			}
+		}
+	}
+
 	if (!collisions.length) {
 		return;
 	}
@@ -81,10 +109,9 @@ const warnOnCliOptionCollisions = (
 		? definition.name[0]
 		: definition.name;
 	logger.warn(
-		`Command '${commandName}' declares option(s) ${collisions
-			.map((name) => `'--${name}'`)
-			.join(", ")} that the CLI already defines globally. The command's ` +
-			`declaration wins while the command runs; rename them to avoid it.`,
+		`Command '${commandName}' declares options that collide with CLI-wide ` +
+			`ones: ${collisions.join("; ")}. The command's declaration wins while ` +
+			`the command runs; rename them to avoid it.`,
 	);
 };
 
@@ -113,12 +140,22 @@ export function createCommandFromDefinition<
 		? targetInjector.resolve("options")
 		: null;
 
-	warnOnCliOptionCollisions(
-		targetInjector,
-		definition,
-		optionNames,
-		optionsService,
-	);
+	warnOnCliOptionCollisions(targetInjector, definition, schema, optionsService);
+
+	const commandName = Array.isArray(definition.name)
+		? definition.name[0]
+		: definition.name;
+
+	const fail = (message: string): never => {
+		if (typeof message !== "string" || !message.trim()) {
+			throw new Error(
+				`ctx.fail() for command '${commandName}' requires a non-empty message.`,
+			);
+		}
+
+		const errors: IErrors = targetInjector.resolve("errors");
+		return errors.failWithHelp(message);
+	};
 
 	// Read per call rather than snapshotted here: the options service only holds
 	// this command's parsed values once validateOptions has run for it.
@@ -128,7 +165,7 @@ export function createCommandFromDefinition<
 			options[optionName] = optionsService[optionName];
 		}
 
-		return { args, options };
+		return { args, options, fail };
 	};
 
 	const acceptsArguments = definition.arguments === "any";
@@ -144,10 +181,7 @@ export function createCommandFromDefinition<
 			: { enableHooks: definition.enableHooks }),
 		canExecute: async (args: string[]): Promise<boolean> => {
 			if (!acceptsArguments && args.length) {
-				targetInjector
-					.resolve("errors")
-					.failWithHelp("This command doesn't accept parameters.");
-				return false;
+				fail("This command doesn't accept parameters.");
 			}
 
 			const refine = definition.canExecute;

test/define-command.ts

@@ -496,26 +496,58 @@ describe("defineCommand", () => {
 			assert.deepEqual(command.dashedOptions, {});
 		});
 
-		it("warns when a declared option shadows a CLI-wide one", () => {
+		it("warns when a declared option or alias shadows a CLI-wide one", () => {
 			const testInjector = createTestInjector({
-				options: { verbose: { type: "boolean" } },
+				options: {
+					verbose: { type: "boolean" },
+					path: { type: "string", alias: "p" },
+				},
 			});
 
 			createCommandFromDefinition(
 				defineCommand({
 					name: "dctestshadow",
-					options: { verbose: booleanOption(), fresh: booleanOption() },
+					options: {
+						verbose: booleanOption(),
+						output: stringOption({ alias: ["p", "o"] }),
+						fresh: booleanOption({ alias: "f" }),
+					},
 					run: (): void => undefined,
 				}),
 				testInjector,
 			);
 
 			const logger: LoggerStub = testInjector.resolve("logger");
-			assert.match(
+			assert.include(
+				logger.warnOutput,
+				"'--verbose' with the CLI option '--verbose'",
+			);
+			assert.include(
 				logger.warnOutput,
-				/Command 'dctestshadow' declares option\(s\) '--verbose' that the CLI already defines globally/,
+				"alias '-p' of '--output' with the CLI option '--path'",
 			);
 			assert.notInclude(logger.warnOutput, "--fresh");
+			assert.notInclude(logger.warnOutput, "'-o'");
+		});
+
+		it("stays quiet when nothing collides", () => {
+			const testInjector = createTestInjector({
+				options: { path: { type: "string", alias: "p" } },
+			});
+
+			createCommandFromDefinition(
+				defineCommand({
+					name: "dctestnoshadow",
+					options: { output: stringOption({ alias: ["o", "out"] }) },
+					run: (): void => undefined,
+				}),
+				testInjector,
+			);
+
+			assert.strictEqual(
+				(<LoggerStub>testInjector.resolve("logger")).warnOutput,
+				"",
+			);
 		});
 	});
 
@@ -617,6 +649,81 @@ describe("defineCommand", () => {
 		});
 	});
 
+	describe("ctx.fail", () => {
+		const createFailInjector = (): IInjector => {
+			const testInjector = createTestInjector();
+			testInjector.register("errors", {
+				failWithHelp: (message: string) => {
+					throw new Error(`with help: ${message}`);
+				},
+			});
+			return testInjector;
+		};
+
+		it("fails the command from run, through failWithHelp", async () => {
+			const command = createCommandFromDefinition(
+				defineCommand({
+					name: "dctestfailrun",
+					run: (ctx) => ctx.fail("no project found"),
+				}),
+				createFailInjector(),
+			);
+
+			await assert.isRejected(
+				command.execute([]),
+				/with help: no project found/,
+			);
+		});
+
+		it("fails the command from canExecute, through failWithHelp", async () => {
+			const command = createCommandFromDefinition(
+				defineCommand({
+					name: "dctestfailcan",
+					arguments: "any",
+					canExecute: (ctx) =>
+						ctx.args.length === 1 || ctx.fail("expected one argument"),
+					run: (): void => undefined,
+				}),
+				createFailInjector(),
+			);
+
+			assert.isTrue(await command.canExecute(["one"]));
+			await assert.isRejected(
+				command.canExecute([]),
+				/with help: expected one argument/,
+			);
+		});
+
+		it("rejects a message that carries nothing", async () => {
+			const command = createCommandFromDefinition(
+				defineCommand({
+					name: "dctestfailempty",
+					run: (ctx) => ctx.fail("  "),
+				}),
+				createFailInjector(),
+			);
+
+			await assert.isRejected(
+				command.execute([]),
+				/ctx.fail\(\) for command 'dctestfailempty' requires a non-empty message/,
+			);
+		});
+
+		it("still lets a thrown error through unchanged", async () => {
+			const command = createCommandFromDefinition(
+				defineCommand({
+					name: "dctestthrow",
+					run: () => {
+						throw new Error("raw failure");
+					},
+				}),
+				createFailInjector(),
+			);
+
+			await assert.isRejected(command.execute([]), /^raw failure$/);
+		});
+	});
+
 	describe("command flags", () => {
 		it("passes disableAnalytics and enableHooks through", () => {
 			const command = createCommandFromDefinition(

test/type-fixtures/define-command-types.ts

@@ -55,6 +55,8 @@ defineCommand({
 	name: "typefixture|no-options",
 	run(ctx) {
 		expectExactType<IsExact<typeof ctx.args, string[]>>();
+		// `never` is what lets fail() end a branch without a return.
+		expectExactType<IsExact<ReturnType<typeof ctx.fail>, never>>();
 
 		// @ts-expect-error - nothing is declared, so any access is a typo
 		ctx.options.anything;