-
Notifications
You must be signed in to change notification settings - Fork 33
feat: use undefined
as fallback for string
& number
args with no values
#195
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
=======================================
Coverage ? 94.10%
=======================================
Files ? 7
Lines ? 509
Branches ? 166
=======================================
Hits ? 479
Misses ? 30
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
test/parser.test.ts
Outdated
|
||
it("handles arguments with no values", () => { | ||
const args = ["--name", "--age"]; | ||
const opts = { string: ["name"], number: ["age"] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an (additional) test that ensures (by default) --flag
sets to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
src/_parser.ts
Outdated
: ((x = +val), x * 0 === 0) | ||
? x | ||
: val; | ||
: ~opts.number.indexOf(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~-1
== 0
while is valid, is really not readable we should use explicit -1
check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I replaced indexOf
with includes
and also updated other instances of ~
.
src/_parser.ts
Outdated
@@ -8,6 +8,7 @@ type Default = Dict<any>; | |||
export interface Options { | |||
boolean?: Arrayable<string>; | |||
string?: Arrayable<string>; | |||
number?: Arrayable<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also to be honest i'm not sure if we should add string to parser level. there is need to distingoush between undefined|string|boolean but number is derived from string in upper layer by parsing string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe we need to add all the booleans into strings for processing.
For example:
opts.string = toArr(opts.string).concat(toArr(opts.number));
This could make the parser simpler. WDYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Removed redundant processing here as the conversion from string to number has already been handled in upper layer.
resolves #142
This PR changes the fallback for
string
&number
args from""
toundefined
.This change enables differentiation between user-specified values, such as:
test --config
vstest --config ""
Example:
Previously:
test --name
=>ctx.args.name === ""
test --name ""
=>ctx.args.name === ""
test --age
=> throwsNow:
test --name
=>ctx.args.name === undefined
&("name" in ctx.args) === true
test --name ""
=>ctx.args.name === ""
test --age
=>ctx.args.age === undefined
&("age" in ctx.args) === true