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

Coalesce boolean arguments to actual Boolean type instead of string #763

Closed
wants to merge 5 commits into from

Conversation

joshuayoes
Copy link

@joshuayoes joshuayoes commented Aug 25, 2022

When we pass --key=false flag arguments to gluegun, we get { options: { key: 'false' } } instead of { options: { key: 'false' } }.

This isn't how I expect to consume these in commands. This can handled on the program level, but I feel like that it should be handled on the gluegun level since I can't imagine a use case where we'd want true or false to be a string

@joshuayoes
Copy link
Author

joshuayoes commented Aug 25, 2022

I've added failing tests to demonstrate desired behavior, you can see a sample of those failed test runs here

@joshuayoes
Copy link
Author

Only problem I see is that this would definitely be a breaking change in my eyes

@joshuayoes
Copy link
Author

It also may be better to take a PR like this, where we could provide the yargs-parser object optionally, and then allow for opting into coalescing options

@joshuayoes
Copy link
Author

Got some input from Jamon. Gluegun already doesn't have great type-safety guarantees on command parsing, so this quality of life improvement would be worth a potentially breaking change

@joshuayoes joshuayoes self-assigned this Aug 25, 2022
@joshuayoes joshuayoes marked this pull request as ready for review August 25, 2022 18:45
@joshuayoes
Copy link
Author

Screen Shot 2022-08-25 at 4 43 04 PM
https://dev.azure.com/infinitered/gluegun/_build?view=runs

I think something changed about ts-node or typescript dependency between now and May.

I don't think these changes are causing this issue, but I will investigate more before merging

Copy link
Member

@jamonholmgren jamonholmgren left a comment

Choose a reason for hiding this comment

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

Good to go (after CI fixed)

I'll merge & release once CI is fixed and you slim down that helper function.

@@ -1,6 +1,7 @@
import { GluegunParameters } from '../domain/toolbox'
import { Options } from '../domain/options'
import { equals, is } from './utils'
import type yargsParser from 'yargs-parser'
Copy link
Member

Choose a reason for hiding this comment

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

Smart to use an import type here.

Comment on lines +36 to +45
Object.entries(parsed).map(([key, value]) => {
// if value is 'true' or 'false', convert to boolean
if (value === 'true') {
return [key, true]
}
if (value === 'false') {
return [key, false]
}
return [key, value]
}),
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the simplified version here that I mentioned in the related Ignite PR.

@jamonholmgren
Copy link
Member

Merged into #783 since this will be a breaking change too.

@jamonholmgren jamonholmgren deleted the feat/normalize-flag-values branch September 28, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants