-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(src): generic exponent plugin #91
Conversation
}; | ||
|
||
const validateNumericString = (str: string) => { | ||
if (isNaN(+Number(str))) { |
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.
the condition will be more readable with this typeof str !== 'number'
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.
cool. also added a unit test to cover this scenario.
I have to say this was a bit counter intuitive for me: if we pass str as str: string
, how come typeof str !== 'number'
isn't always true? guess it's one of those unique TypeScript things...
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.
I don't think this change works.
typeof str !== 'number'
does not check that the string is numeric, it checks whether the type is a number.
Depending on the purpose of the check, you might want to check that the string parses to a number, or use some regex to check that the string only contains numbers.
@manushak please confirm
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.
you're right @jmcook1186 , but shouldn't we require the user to pass a number instead of a string, like this: '10'?
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.
@jmcook1186 your turn :)
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.
Hi @pazbardanl @manushak we should be able to parse numeric strings too. Can you confirm that this is the current behaviour.
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.
In this case @pazbardanl, please, use your version
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
0213323
to
df2b1f9
Compare
df2b1f9
to
92ee329
Compare
Signed-off-by: Paz Barda <[email protected]>
92ee329
to
b9e5d4f
Compare
Signed-off-by: Joseph Cook <[email protected]>
Signed-off-by: Joseph Cook <[email protected]>
Types of changes
A description of the changes proposed in the Pull Request