Skip to content

wire format for a nonEmpty field in string/[]string params #1912

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.

Sign up for GitHub

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

Open
Berlioz wants to merge 2 commits into
base: master
Choose a base branch
from
Open

wire format for a nonEmpty field in string/[]string params #1912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce16563
wire format for a nonEmpty field in string/[]string params
Berlioz Jun 17, 2026
3bf4abf
missing comment
Berlioz Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions spec/fixtures/sources/commonjs-params/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const params = require("../../../../src/params");

params.defineString("BORING");
const foo = params.defineString("FOO", { input: { text: { validationRegex: "w+" } } });
const bar = params.defineString("BAR", { default: foo, label: "asdf" });
const bar = params.defineString("BAR", { default: foo, label: "asdf", nonEmpty: true });
params.defineString("BAZ", { input: { select: { options: [{ value: "a" }, { value: "b" }] } } });

params.defineInt("AN_INT", { default: bar.equals("qux").thenElse(0, 1) });
Expand All @@ -21,7 +21,7 @@ params.defineInt("ANOTHER_INT", {
},
});

params.defineList("LIST_PARAM", {input: { multiSelect: { options: [{ value: "c" }, { value: "d" }, { value: "e" }]}}})
params.defineList("LIST_PARAM", {nonEmpty: true, input: { multiSelect: { options: [{ value: "c" }, { value: "d" }, { value: "e" }]}}})

params.defineSecret("SUPER_SECRET_FLAG");

Expand Down
2 changes: 2 additions & 0 deletions spec/runtime/loader.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ describe("loadStack", () => {
type: "string",
default: "{{ params.FOO }}",
label: "asdf",
nonEmpty: true,
},
{
name: "BAZ",
Expand All @@ -429,6 +430,7 @@ describe("loadStack", () => {
{
name: "LIST_PARAM",
type: "list",
nonEmpty: true,
input: {
multiSelect: { options: [{ value: "c" }, { value: "d" }, { value: "e" }] },
},
Expand Down
5 changes: 3 additions & 2 deletions src/params/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
SecretParamOptions,
InternalExpression,
InterpolationExpression,
SupportsNonEmpty,
} from "./types";
Comment on lines 39 to 43

@gemini-code-assist gemini-code-assist Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since SupportsNonEmpty is no longer needed as a separate type, we can remove its import.

  SecretParamOptions,
  InternalExpression,
  InterpolationExpression,
} from "./types";


export { BUCKET_PICKER, select, multiSelect } from "./types";
Expand Down Expand Up @@ -181,7 +182,7 @@
* @param options Configuration options for the parameter.
* @returns A parameter with a `string` return type for `.value`.
*/
export function defineString(name: string, options: ParamOptions<string> = {}): StringParam {
export function defineString(name: string, options: ParamOptions<string> & SupportsNonEmpty = {}): StringParam {

Check failure on line 185 in src/params/index.ts

View workflow job for this annotation

GitHub Actions / lint (22.x) 

Replace `name:·string,·options:·ParamOptions<string>·&·SupportsNonEmpty·=·{}` with `⏎··name:·string,⏎··options:·ParamOptions<string>·&·SupportsNonEmpty·=·{}⏎`

Check failure on line 185 in src/params/index.ts

View workflow job for this annotation

GitHub Actions / lint (24.x) 

Replace `name:·string,·options:·ParamOptions<string>·&·SupportsNonEmpty·=·{}` with `⏎··name:·string,⏎··options:·ParamOptions<string>·&·SupportsNonEmpty·=·{}⏎`
const param = new StringParam(name, options);
registerParam(param);
return param;
Comment on lines +185 to 188

@gemini-code-assist gemini-code-assist Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

With the conditional ParamOptions<T> type, defineString no longer needs to manually intersect with SupportsNonEmpty.

Suggested change
export function defineString(name: string, options: ParamOptions<string> & SupportsNonEmpty = {}): StringParam {
const param = new StringParam(name, options);
registerParam(param);
return param;
export function defineString(name: string, options: ParamOptions<string> = {}): StringParam {
const param = new StringParam(name, options);
registerParam(param);
return param;

Expand Down Expand Up @@ -235,7 +236,7 @@
* @param options Configuration options for the parameter.
* @returns A parameter with a `string[]` return type for `.value`.
*/
export function defineList(name: string, options: ParamOptions<string[]> = {}): ListParam {
export function defineList(name: string, options: ParamOptions<string[]> & SupportsNonEmpty = {}): ListParam {

Check failure on line 239 in src/params/index.ts

View workflow job for this annotation

GitHub Actions / lint (22.x) 

Replace `name:·string,·options:·ParamOptions<string[]>·&·SupportsNonEmpty·=·{}` with `⏎··name:·string,⏎··options:·ParamOptions<string[]>·&·SupportsNonEmpty·=·{}⏎`

Check failure on line 239 in src/params/index.ts

View workflow job for this annotation

GitHub Actions / lint (24.x) 

Replace `name:·string,·options:·ParamOptions<string[]>·&·SupportsNonEmpty·=·{}` with `⏎··name:·string,⏎··options:·ParamOptions<string[]>·&·SupportsNonEmpty·=·{}⏎`
const param = new ListParam(name, options);
registerParam(param);
return param;
Comment on lines +239 to 242

@gemini-code-assist gemini-code-assist Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

With the conditional ParamOptions<T> type, defineList no longer needs to manually intersect with SupportsNonEmpty.

Suggested change
export function defineList(name: string, options: ParamOptions<string[]> & SupportsNonEmpty = {}): ListParam {
const param = new ListParam(name, options);
registerParam(param);
return param;
export function defineList(name: string, options: ParamOptions<string[]> = {}): ListParam {
const param = new ListParam(name, options);
registerParam(param);
return param;

Expand Down
13 changes: 10 additions & 3 deletions src/params/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,8 @@
input?: ParamInput<T>;
/** Optional format annotation for additional type information (e.g., "json" for JSON-encoded secrets). */
format?: string;
/** Disallows the empty string/empty list when prompting for input. */
nonEmpty?: boolean;
};

/**
Expand All @@ -429,13 +431,18 @@
type: ParamValueType;
input?: ParamInput<T>;
format?: string;
nonEmpty?: boolean;
};

/** Configuration options which can be used to customize the prompting behavior of a parameter. */
/** Basic configuration options which can be used to customize the prompting behavior of a parameter. */
export type ParamOptions<T extends string | number | boolean | string[]> = Omit<
ParamSpec<T>,
"name" | "type"
"name" | "type" | "nonEmpty"
>;
/** A configuration to disallow empty string/empty list, which is unioned to Paramoptions for types where it has meaning. */
export type SupportsNonEmpty = {
nonEmpty?: boolean;
}

Check failure on line 445 in src/params/types.ts

View workflow job for this annotation

GitHub Actions / lint (22.x) 

Insert `;`

Check failure on line 445 in src/params/types.ts

View workflow job for this annotation

GitHub Actions / lint (24.x) 

Insert `;`
Comment on lines +437 to +445

@gemini-code-assist gemini-code-assist Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of creating a separate SupportsNonEmpty type and manually intersecting it in multiple places, we can make ParamOptions<T> conditionally include nonEmpty only when T is a string or string[]. This improves type safety by preventing nonEmpty from being allowed on other parameter types (like IntParam or BooleanParam) and simplifies the function signatures.

Suggested change
/** Basic configuration options which can be used to customize the prompting behavior of a parameter. */
export type ParamOptions<T extends string | number | boolean | string[]> = Omit<
ParamSpec<T>,
"name" | "type"
"name" | "type" | "nonEmpty"
>;
/** A configuration to disallow empty string/empty list, which is unioned to Paramoptions for types where it has meaning. */
export type SupportsNonEmpty = {
nonEmpty?: boolean;
}
/** Basic configuration options which can be used to customize the prompting behavior of a parameter. */
export type ParamOptions<T extends string | number | boolean | string[]> = Omit<
ParamSpec<T>,
"name" | "type" | "nonEmpty"
> & (T extends string | string[] ? { nonEmpty?: boolean } : {});

@Berlioz Berlioz Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Typescript knowers: does this work? Is this remotely readable? I think I blacked out when I read this and saw a vision of Cthulhu as I was under, but maybe I'm not sufficiently rocq-brained.

@inlined inlined Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this works. There's other ways too.


/** Configuration options which can be used to customize the behavior of a secret parameter. */
export interface SecretParamOptions {
Expand All @@ -453,7 +460,7 @@
export abstract class Param<T extends string | number | boolean | string[]> extends Expression<T> {
static type: ParamValueType = "string";

constructor(readonly name: string, readonly options: ParamOptions<T> = {}) {
constructor(readonly name: string, readonly options: ParamOptions<T> & SupportsNonEmpty = {}) {
super();
}
Comment on lines +463 to 465

@gemini-code-assist gemini-code-assist Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

With ParamOptions<T> conditionally including nonEmpty based on T, we can simplify the Param constructor signature to just use ParamOptions<T> instead of intersecting it with SupportsNonEmpty.

Suggested change
constructor(readonly name: string, readonly options: ParamOptions<T> & SupportsNonEmpty = {}) {
super();
}
constructor(readonly name: string, readonly options: ParamOptions<T> = {}) {
super();
}


Expand Down
Loading