-
Notifications
You must be signed in to change notification settings - Fork 565
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(parse): reject numeric strings with non-numbers #2729
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2729 +/- ##
=======================================
Coverage ? 61.40%
=======================================
Files ? 539
Lines ? 27513
Branches ? 6718
=======================================
Hits ? 16893
Misses ? 10620
Partials ? 0 Continue to review full report at Codecov.
|
16721c9
to
817af54
Compare
06e15fd
to
1fee7ae
Compare
This updates the number parsing utilities to reject strings like "1A", which `parseFloat` would ordinarily happily accept as the value `1`.
1fee7ae
to
55bdaaa
Compare
|
||
const parseNumber = (value: string): number => { | ||
const matches = value.match(NUMBER_REGEX); | ||
if (matches === null || matches[0].length !== value.length) { |
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 anchor the regex so you can just use RegExp.test
and not have to check the length of the match?
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.
hahaha your faith in js is heartwarming, no it'll match partials anyway
// * Exponent indicated by a case-insensitive 'E' optionally followed by a | ||
// positive/negative sign and some number of digits. | ||
// It also matches both positive and negative infinity as well and explicit NaN. | ||
const NUMBER_REGEX = /(-?(?:0|[1-9]\d*)(?:\.\d+)?(?:[eE][+-]?\d+)?)|(-?Infinity)|(NaN)/g; |
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.
Very nit: Is this too strict? parseFloat()
handles strings leading with "0" just fine.
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.
It might be overkill at this point (I think the concern was stuff like this) but we can always loosen this later. It's much more difficult to tighten it.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue
N/A
Description
This updates the number parsing utilities to reject strings like "1A", which
parseFloat
would ordinarily happily accept as the value1
. It also updates it to reject the wide swath of alternative number formats that JS numbers can have.Testing
Additional unit tests were added.
Additional context
Add any other context about the PR here.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.