-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add coercion utils #38
Conversation
I'm not sure we need those functions |
@ritave We need them for the upcoming |
); | ||
}); | ||
|
||
it.each([true, false, null, undefined, NaN, {}, []])( |
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.
What happens with negative zero?
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.
console.log(createNumber(-0)); // -0
console.log(createNumber('-0')); // -0
console.log(createNumber(BigInt(-0))); // 0
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.
Do you think it is worth adding tests for these cases?
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's literally 1 line change, why not.
But not that important if you want to just merge
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!
* This validates that the value is a number-like value, and that the resulting | ||
* number is not `NaN` or `Infinity`. | ||
* | ||
* @example |
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.
Really loving the examples. We should do more of this in JSDocs.
After some thinking about it, and talking about it with some people, I decided to make the coercions more strict. Rather than guessing what the user meant, they only accept well defined values now. For example, the hex/byte coercions only accept hexadecimal strings, if they start with "0x" (or "0X"). This avoids situations like "should we parse |
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.
Good call, these new changes look good.
import { add0x } from './hex'; | ||
import { bytesToHex, hexToBytes } from './bytes'; | ||
|
||
describe('createNumber', () => { |
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.
Should we test that this accepts 0x-prefixed hex strings? And also treats strings that look like numbers as decimals?
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.
Lines 17 to 18 in d77e1c5
expect(createNumber(add0x(value.toString(16)))).toBe(value); | |
expect(createNumber(value.toString(10))).toBe(value); |
This adds utils for coercing values to a standardised value, e.g., a number or string to
bigint
, or a hexadecimal string to a byte array, with some simple utility functions.