WIP: nonEmpty param option support#10678
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'nonEmpty' option to parameter definitions to reject empty strings and empty arrays during CLI prompting, propagating this configuration through various prompt helper functions. The review feedback highlights a critical TypeScript compilation error caused by changing the signature of 'promptResourceString' (which breaks an existing caller), and suggests correcting an inaccurate error message in 'promptSelectMultiple' when no options are selected.
inlined
left a comment
inlined
left a comment
There was a problem hiding this comment.
I think I recommend that nonEmpty is a part of input? we can talk offline.
| prompt += ` \n(${param.description})`; | ||
| } | ||
| return promptText<boolean>(prompt, param.input, resolvedDefault, isTruthyInput); | ||
| return promptText<boolean>(prompt, param.input, resolvedDefault, false, isTruthyInput); |
There was a problem hiding this comment.
please always leave a comment on any boolean literal ever used as a parameter
| prompt: string, | ||
| input: ResourceInput, | ||
| projectId: string, | ||
| disallowEmpty: boolean | undefined, |
There was a problem hiding this comment.
why disallowEmpty and nonEmpty? Also, why mandatory but supports undefined?
There was a problem hiding this comment.
Yeah (naming aside) this could just be disallowEmpty: boolean = false so there's a default value for the case where we have undefined from it not being in functions.yaml right? (same elsewhere).
I have a hunch on why the name change from nonEmpty to disallowEmpty but I'd like to hear Victor's answer :) ... I do think it's a debateable shift.
| input?: ParamInput<T>; | ||
|
|
||
| // Reject the empty string and empty array as valid input. | ||
| nonEmpty?: boolean; |
There was a problem hiding this comment.
I wonder how hard it is to make the field only present for the right types. At minimum you can say:
type TypeIfExtends<Result, Test, Base> = Test extends Base ? Result : never;
// ...
nonEmpty: TypeIfExtends<boolean, T, string | string[]>
| prompt, | ||
| param.input, | ||
| resolvedDefault, | ||
| param.nonEmpty, |
There was a problem hiding this comment.
why is nonEmpty not part of input?
There was a problem hiding this comment.
If it is (which from what I understand with a bit of research I would support) do we also need to update the API proposal to be clear about that?
Or we enforce nonEmpty at deployment time and not just input prompting time?
I guess a difference between functions and extensions is users can edit params after the input form by hand editing .env files (or could add nonEmpty after a first deployment that writes an empty string).
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Chiming in with a few thoughts since I went through this PR trying to understand it. But overall, I think just having Thomas's LG (and not blocking on more review from me) is fine.
| prompt: string, | ||
| input: ResourceInput, | ||
| projectId: string, | ||
| disallowEmpty: boolean | undefined, |
There was a problem hiding this comment.
Yeah (naming aside) this could just be disallowEmpty: boolean = false so there's a default value for the case where we have undefined from it not being in functions.yaml right? (same elsewhere).
I have a hunch on why the name change from nonEmpty to disallowEmpty but I'd like to hear Victor's answer :) ... I do think it's a debateable shift.
| prompt, | ||
| param.input, | ||
| resolvedDefault, | ||
| param.nonEmpty, |
There was a problem hiding this comment.
If it is (which from what I understand with a bit of research I would support) do we also need to update the API proposal to be clear about that?
Or we enforce nonEmpty at deployment time and not just input prompting time?
I guess a difference between functions and extensions is users can edit params after the input form by hand editing .env files (or could add nonEmpty after a first deployment that writes an empty string).
4 participants
Also gets rid of that immutable copy-paste mistake which has never done anything.