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

feat(stdlib): Number.parseInt #1051

Merged
merged 4 commits into from
Dec 8, 2021
Merged

feat(stdlib): Number.parseInt #1051

merged 4 commits into from
Dec 8, 2021

Conversation

ospencer
Copy link
Member

@ospencer ospencer commented Dec 7, 2021

What it says on the tin.

@ospencer ospencer requested a review from a team December 7, 2021 01:16
@ospencer ospencer self-assigned this Dec 7, 2021
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Loving it ❤️ A few things

compiler/test/stdlib/number.test.gr Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/runtime/stringUtils.gr Outdated Show resolved Hide resolved
let radix = WasmI32.fromGrain(radix)
let result = if (WasmI32.eqz(radix & Tags._GRAIN_NUMBER_TAG_MASK) || radix < WasmI32.fromGrain(2) || radix > WasmI32.fromGrain(36)) {
Memory.incRef(WasmI32.fromGrain(Err))
Err("Radix must be an integer between 2 and 36")
Copy link
Member

Choose a reason for hiding this comment

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

Do we want exception types for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only reason I'm a no is because we don't have a great way to re-export exceptions right now. I think there's an issue for that.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open an issue to do this once that issue is solved? Also verify that issue exists 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

stdlib/runtime/stringUtils.gr Show resolved Hide resolved
ospencer and others added 3 commits December 6, 2021 20:43
Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Approved! Awesome work 🍻

Copy link
Member

@marcusroberts marcusroberts left a comment

Choose a reason for hiding this comment

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

Looks excellent!

@ospencer ospencer merged commit abafb58 into main Dec 8, 2021
@ospencer ospencer deleted the oscar/parseint branch December 8, 2021 18:09
@giluis
Copy link

giluis commented Mar 29, 2022

Hello everyone, new here.
In the tests in number.test.gr why use assert Number.parseInt("ZYXW44AB", 36) == Ok(2818805666483) for Ok results, but assert Result.isErr(Number.parseInt("", 10)) for Err results?

Wouldn't assert Number.parseInt("abc",10) == Err("Some error message") make more sense consistency-wise (if the language allows for this, not sure it does)?

@phated
Copy link
Member

phated commented Mar 29, 2022

Hey @giluis - the reason for the tests written in that fashion is that you shouldn't pattern match on a string inside of Err. The string is just an error message and isn't considered during semantic versioning.

@github-actions github-actions bot mentioned this pull request May 31, 2022
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

4 participants