Skip to content

Made boolean CLI option values optional#446

Merged
Perryvw merged 1 commit intomasterfrom
feature/cli-boolean-optional
Feb 23, 2019
Merged

Made boolean CLI option values optional#446
Perryvw merged 1 commit intomasterfrom
feature/cli-boolean-optional

Conversation

@Perryvw
Copy link
Copy Markdown
Member

@Perryvw Perryvw commented Feb 23, 2019

If no value is supplied, the value defaults to true.

@Perryvw Perryvw requested a review from tomblind February 23, 2019 14:56

if (option.type === "boolean" && (argument === undefined || argument.startsWith("-"))) {
// Set boolean arguments without supplied value to true
return { isValid: true, result: true, increment: 0 };
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would it be better for result to be option.default here (in case a flag option is added that defaults to true if not specified)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess it should be !option.default? Otherwise setting the option without value would be the same as not setting it at all. I'm a little concerned that's just confusing. Would it ever make sense to have an option that is true by default and can be set to false by just adding its name to CLI? I'm not sure that is a valid use case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, actually you're probably right. If you set a flag up like that, it could just be inverted.

@Perryvw Perryvw merged commit f7e95b9 into master Feb 23, 2019
@Perryvw Perryvw deleted the feature/cli-boolean-optional branch February 23, 2019 17:17
hazzard993 pushed a commit to hazzard993/TypescriptToLua that referenced this pull request Feb 26, 2019
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