Skip to content

Conversation

waldekmastykarz
Copy link
Member

No description provided.

Copy link
Contributor

@MartinM85 MartinM85 left a comment

Choose a reason for hiding this comment

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

Quick review. Nothing major, but maybe I overlooked something.

.strict();
export const options = z.strictObject({
...appCommandOptions.shape,
preview: z.boolean().optional().default(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

General remark.
If the option is optional, I would expect that the type will be boolean | undefined, but in that case the return type is simple boolean. It's a little bit confusing for me.
Is it valid to define option like preview: z.boolean().default(false).optional()?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a matter of preference/need, whether we want to distinguish between undefined and false or not. In some case we do, like for set commands where we want to be able to tell whether to change a property to disabled (false) or leave it unmodified (undefined). For other things, we might not care about the undefined state and consider it equal to false for which we specify a default on the boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add an example how to define flag option? I'm not sure if it's required to set default value to false or keep it only optional
flag: z.boolean().optional() vs flag: z.boolean().optional().default(false). I would prefer flag: z.boolean().optional(), but global option like the verbose is defined like flag: z.boolean().optional().default(false).

Copy link
Member Author

Choose a reason for hiding this comment

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

.strict();
export const options = z.strictObject({
...globalOptionsZod.shape,
userId: z.string().refine(id => validation.isValidGuid(id), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
userId: z.string().refine(id => validation.isValidGuid(id), {
userId: z.uuid().refine(id => validation.isValidGuid(id), {

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're using uuid(), then we don't need to add manual refinement on top. z.uuid() would suffice.

@waldekmastykarz
Copy link
Member Author

Thanks for the feedback @MartinM85! Very keen eye!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants