-
Notifications
You must be signed in to change notification settings - Fork 381
Adds prompting on invalid option sets in Zod-enabled commands #7061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ import os from 'os'; | |||||||||||||||||||||||||||||||||||||||
| import path from 'path'; | ||||||||||||||||||||||||||||||||||||||||
| import { fileURLToPath, pathToFileURL } from 'url'; | ||||||||||||||||||||||||||||||||||||||||
| import yargs from 'yargs-parser'; | ||||||||||||||||||||||||||||||||||||||||
| import { ZodError } from 'zod'; | ||||||||||||||||||||||||||||||||||||||||
| import z, { ZodError } from 'zod'; | ||||||||||||||||||||||||||||||||||||||||
| import Command, { CommandArgs, CommandError } from '../Command.js'; | ||||||||||||||||||||||||||||||||||||||||
| import GlobalOptions from '../GlobalOptions.js'; | ||||||||||||||||||||||||||||||||||||||||
| import config from '../config.js'; | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -186,15 +186,35 @@ async function execute(rawArgs: string[]): Promise<void> { | |||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||
| const hasNonRequiredErrors = result.error.issues.some(i => i.code !== 'invalid_type'); | ||||||||||||||||||||||||||||||||||||||||
| const shouldPrompt = cli.getSettingWithDefaultValue<boolean>(settingsNames.prompt, true); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (hasNonRequiredErrors === false && | ||||||||||||||||||||||||||||||||||||||||
| shouldPrompt) { | ||||||||||||||||||||||||||||||||||||||||
| if (!shouldPrompt) { | ||||||||||||||||||||||||||||||||||||||||
| result.error.issues.forEach(e => { | ||||||||||||||||||||||||||||||||||||||||
| if (e.code === 'invalid_type' && | ||||||||||||||||||||||||||||||||||||||||
| e.input === undefined) { | ||||||||||||||||||||||||||||||||||||||||
| (e.message as any) = `Required option not specified`; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const missingRequiredValuesErrors: z.core.$ZodIssue[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => (e.code === 'invalid_type' && e.input === undefined) || | ||||||||||||||||||||||||||||||||||||||||
| (e.code === 'custom' && e.params?.customCode === 'required')); | ||||||||||||||||||||||||||||||||||||||||
| const optionSetErrors: z.core.$ZodIssueCustom[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => e.code === 'custom' && e.params?.customCode === 'optionSet') as z.core.$ZodIssueCustom[]; | ||||||||||||||||||||||||||||||||||||||||
| const otherErrors: z.core.$ZodIssue[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => !missingRequiredValuesErrors.includes(e) && !optionSetErrors.includes(e as z.core.$ZodIssueCustom)); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (otherErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
Jwaegebaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (missingRequiredValuesErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| await cli.error('🌶️ Provide values for the following parameters:'); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| for (const issue of result.error.issues) { | ||||||||||||||||||||||||||||||||||||||||
| const optionName = issue.path.join('.'); | ||||||||||||||||||||||||||||||||||||||||
| for (const error of missingRequiredValuesErrors) { | ||||||||||||||||||||||||||||||||||||||||
| const optionName = error.path.join('.'); | ||||||||||||||||||||||||||||||||||||||||
| const optionInfo = cli.commandToExecute.options.find(o => o.name === optionName); | ||||||||||||||||||||||||||||||||||||||||
| const answer = await cli.promptForValue(optionInfo!); | ||||||||||||||||||||||||||||||||||||||||
| // coerce the answer to the correct type | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -206,15 +226,14 @@ async function execute(rawArgs: string[]): Promise<void> { | |||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(e.message, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||
| result.error.issues.forEach(i => { | ||||||||||||||||||||||||||||||||||||||||
| if (i.code === 'invalid_type' && | ||||||||||||||||||||||||||||||||||||||||
| i.input === undefined) { | ||||||||||||||||||||||||||||||||||||||||
| (i.message as any) = `Required option not specified`; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (optionSetErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| for (const error of optionSetErrors) { | ||||||||||||||||||||||||||||||||||||||||
| await promptForOptionSetNameAndValue(cli.optionsFromArgs, error.params?.options); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1057,6 +1076,16 @@ function shouldTrimOutput(output: string | undefined): boolean { | |||||||||||||||||||||||||||||||||||||||
| return output === 'text'; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| async function promptForOptionSetNameAndValue(args: CommandArgs, options: string[]): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||
| await cli.error(`🌶️ Please specify one of the following options:`); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const selectedOptionName = await prompt.forSelection<string>({ message: `Option to use:`, choices: options.map((choice: any) => { return { name: choice, value: choice }; }) }); | ||||||||||||||||||||||||||||||||||||||||
| const optionValue = await prompt.forInput({ message: `${selectedOptionName}:` }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| args.options[selectedOptionName] = optionValue; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| args.options[selectedOptionName] = optionValue; | |
| let coercedValue: unknown = optionValue; | |
| // Basic type coercion to align with how CLI arguments are typically parsed | |
| if (typeof optionValue === 'string') { | |
| const trimmed = optionValue.trim(); | |
| if (trimmed === 'true') { | |
| coercedValue = true; | |
| } | |
| else if (trimmed === 'false') { | |
| coercedValue = false; | |
| } | |
| else if (trimmed !== '' && !isNaN(Number(trimmed))) { | |
| coercedValue = Number(trimmed); | |
| } | |
| } | |
| args.options[selectedOptionName] = coercedValue as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I actually managed to reproduce the type coercion bug. Here's what happens with m365 pp environment get:
- You get prompted to pick an option > select default
- Type something as the value > CLI crashes with Error: The value 'test' for option '--default' is not a valid boolean
- Even if you type true, it loops back and prompts again for the default as if it's still missing
The value gets stored as a raw string ("true") in args.options, but the Zod schema expects a proper boolean true. So validation fails again, this time as a missingRequiredValuesErrors error, which re-prompts you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit nitpicking but I think
.length > 0would be a bit cleaner in this scenarioThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a performance penalty for using length over some. Typically, when you use length, the runtime needs to calculate the full length of a collection. In comparison, when you use some, the runtime uses the iterator and stops as soon as the condition is met, which would be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, it’s a field the engine keeps up to date when the array changes, so checking
otherErrors.length > 0is basically just a quick property read.