Skip to content

Fixes casing and autocomplete for enums based on zod#6373

Closed
waldekmastykarz wants to merge 1 commit intopnp:mainfrom
waldekmastykarz:fix-login2
Closed

Fixes casing and autocomplete for enums based on zod#6373
waldekmastykarz wants to merge 1 commit intopnp:mainfrom
waldekmastykarz:fix-login2

Conversation

@waldekmastykarz
Copy link
Copy Markdown
Member

This PR introduces two changes:

  1. A new zod schema type for using enums, which adds support for case-insensitive parsing of enum values for commands
  2. Building command autocomplete off of enum values rather than names (deviceCode instead of DeviceCode)

Copy link
Copy Markdown
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

While this will work for existing enums like login uses, what about other enum commands?
I think we'll need something similar for all commands that use choices for instance:

export const globalOptionsZod = z.object({
query: z.string().optional(),
output: zod.alias('o', z.enum(['csv', 'json', 'md', 'text', 'none']).optional()),
debug: z.boolean().default(false),
verbose: z.boolean().default(false)
});

@waldekmastykarz
Copy link
Copy Markdown
Member Author

While this will work for existing enums like login uses, what about other enum commands? I think we'll need something similar for all commands that use choices for instance:

export const globalOptionsZod = z.object({
query: z.string().optional(),
output: zod.alias('o', z.enum(['csv', 'json', 'md', 'text', 'none']).optional()),
debug: z.boolean().default(false),
verbose: z.boolean().default(false)
});

I suggest that we migrate all string-based enums to native enums. This will help us make our code more robust. We'll no longer depend on magic strings, and instead will have full design-time support for typings and refactorings.

@Adam-it Adam-it self-assigned this Oct 11, 2024
Copy link
Copy Markdown
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Tested locally and indeed the enum is now case insensitive which is very convenient
image

@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Oct 11, 2024

ready to merge 🚀

@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Oct 13, 2024

Merged manually. Thank you for your awesome contribution 👏.
You Rock 🤩

@Adam-it Adam-it closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants