-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix(cli): Improve number parsing #2454
base: master
Are you sure you want to change the base?
Conversation
6efdc03
to
26a8fd4
Compare
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.
LGTM!
packages/cli/src/number-parse.js
Outdated
if (typeof input === 'number' && !Number.isNaN(input)) { | ||
return input; | ||
} | ||
|
||
if (typeof input === 'string' && /^-?\d+(\.\d+)?$/.test(input.trim())) { | ||
return Number(input); | ||
} |
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.
A thing you can rely on is that Number(string)
will return NaN
if the string doesn’t parse as any valid JavaScript representation of a number
.
You can also assume parseNumber
receives string
and will never see a number
as input, given that this is not a general purpose utility but specifically for parsing numbers for the CLI.
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.
(Notably, this will expand the range of expressible numbers to include 1e3
without requiring any additional complexity in a regex.)
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.
An alternative suggestion that I especially like for CLIs, from #2204:
Line 110 in 20b4d24
const parseNumber = str => (/[0-9]/.test(str || '') ? Number(str) : NaN); |
example valid inputs:
- "42"
- "-40.0"
- ".5"
- "1.337e3"
- "␣+1␣" (where "␣" is a visible representation of whitespace)
- "0B111" (binary for
7
) - "0o040" (octal for
32
) - "0xf00" (hexadecimal for
3840
)
example invalid inputs (i.e., they return NaN
):
- "f00"
- "NaN"
- "Infinity"
- "7up"
- " "
- "␣"
- (empty string)
undefined
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.
Thanks! I also added more test cases based on @gibson042 examples. @kriskowal What is weird is that Number('')
returns 0
instead of NaN
38c2a57
to
3b4a975
Compare
3b4a975
to
4c026c5
Compare
Description
This PR introduces a new
parseNumber
function that safely parses input as a number on cli commands.Additional tests have been added to ensure this new functionality works correctly.
Security Considerations
none
Scaling Considerations
none
Documentation Considerations
none
Testing Considerations
none
Compatibility Considerations
none
Upgrade Considerations
none