Skip to content
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

Merged
merged 4 commits into from Oct 5, 2022
Merged

Add coercion utils #38

merged 4 commits into from Oct 5, 2022

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Oct 3, 2022

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.

@Mrtenz Mrtenz requested a review from a team as a code owner October 3, 2022 13:27
@ritave
Copy link
Member

ritave commented Oct 3, 2022

I'm not sure we need those functions

@Mrtenz
Copy link
Member Author

Mrtenz commented Oct 3, 2022

@ritave We need them for the upcoming abi-utils package at least, and @FrederikBolding and I figured it could be useful for other packages too.

ritave
ritave previously approved these changes Oct 4, 2022
);
});

it.each([true, false, null, undefined, NaN, {}, []])(
Copy link
Member

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?

Copy link
Member Author

@Mrtenz Mrtenz Oct 4, 2022

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

Copy link
Contributor

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?

Copy link
Member

@ritave ritave Oct 5, 2022

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

mcmire
mcmire previously approved these changes Oct 5, 2022
Copy link
Contributor

@mcmire mcmire left a 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
Copy link
Contributor

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.

@Mrtenz Mrtenz dismissed stale reviews from mcmire and ritave via f7688fb October 5, 2022 18:02
@Mrtenz
Copy link
Member Author

Mrtenz commented Oct 5, 2022

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 abcdef as UTF-8 or hex?".

Copy link
Contributor

@mcmire mcmire left a 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', () => {
Copy link
Contributor

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?

Copy link
Member Author

@Mrtenz Mrtenz Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(createNumber(add0x(value.toString(16)))).toBe(value);
expect(createNumber(value.toString(10))).toBe(value);

src/coercers.test.ts Show resolved Hide resolved
@Mrtenz Mrtenz merged commit f241b8a into main Oct 5, 2022
@Mrtenz Mrtenz deleted the mrtenz/coercion-utils branch October 5, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants