Skip to content
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

Unbreak default ints in text input #5118

Merged
merged 6 commits into from Oct 13, 2022
Merged

Unbreak default ints in text input #5118

merged 6 commits into from Oct 13, 2022

Conversation

Berlioz
Copy link
Contributor

@Berlioz Berlioz commented Oct 12, 2022

No description provided.

@Berlioz Berlioz requested a review from taeold October 12, 2022 23:44
Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

Let's add a release note?

Copy link
Member

@inlined inlined left a comment

Choose a reason for hiding this comment

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

Works, but left some nits on style.

@@ -590,7 +590,7 @@ async function promptText<T extends RawParamValue>(
resolvedDefault: T | undefined,
converter: (res: string) => T | retryInput
): Promise<T> {
const res = await promptOnce({
const res: any = await promptOnce({
Copy link
Member

Choose a reason for hiding this comment

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

why not typeof resolvedDefaut | string? Or just call resolvedDefautl.toString() in the prompt call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to do the .toString() thing in this call because I wanted as small a footprint of a change in an emergency patch as possible; I'd agree that it'd probably be just as good/better.

The :any annotation is unnecessary with that tip you gave about string.toString() below, since all it's doing is reassuring tsc that it's not going to have to call never.toString() down below. I'm getting rid of it.

@@ -605,7 +605,7 @@ async function promptText<T extends RawParamValue>(
return promptText<T>(prompt, input, resolvedDefault, converter);
}
}
const converted = converter(res);
const converted = converter(typeof res === "string" ? res : res.toString());
Copy link
Member

Choose a reason for hiding this comment

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

fun fact: string also implements toString so that you can avoid ternaries like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, I knew that, but I apparently didn't know that.

@bkendall bkendall merged commit 12a8887 into master Oct 13, 2022
@bkendall bkendall deleted the fix_blank_fields branch October 13, 2022 22:20
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.

None yet

5 participants