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

Adds BigInt support to stringify util function. #4112

Open
wants to merge 2 commits into
base: master
from

Conversation

@JosejeSinohui
Copy link

JosejeSinohui commented Nov 27, 2019

Description of the Change

Fixes #4090, adds a new case to the _stringify function in utils so it can handle the BigInt case.

Alternate Designs

There were two choices considered in the issue BigInt<123> or 123n i went with the latter because it's the standard way JS presents BigInts

Why should this be in core?

Nice way of representing BigInts in errors.

Benefits

Support for Stringify-ing BigInts

Possible Drawbacks

I had to add /* global BigInt */ at the top of the spec file, i don't know if that is the best place for it.

Applicable issues

Fixes #4090

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Nov 27, 2019

CLA assistant check
All committers have signed the CLA.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 27, 2019

Coverage Status

Coverage increased (+0.07%) to 92.796% when pulling 4c5b742 on JosejeSinohui:issue/4090 into e4e126f on mochajs:master.

@JosejeSinohui

This comment has been minimized.

Copy link
Author

JosejeSinohui commented Nov 27, 2019

I already signed the Contributor License Agreement, i don't know why does it show like pending, is there a way to reset it to sign it again?

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Nov 29, 2019

it could be a mismatch between username / email address.

some ideas:

  • is the email the same in the commits?
  • is the email address and username in the CLA tool the same as your github account?
  • does email address on github account match the email address you used in the tool?
@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Nov 29, 2019

the test should be failing in IE11 and maybe safari... hmm.

@JosejeSinohui

This comment has been minimized.

Copy link
Author

JosejeSinohui commented Dec 4, 2019

the test should be failing in IE11 and maybe safari... hmm.

@boneskull
i checked compatibility here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof

is there something i am missing?

@JosejeSinohui JosejeSinohui force-pushed the JosejeSinohui:issue/4090 branch from 4c5b742 to aca9708 Dec 4, 2019
@JosejeSinohui

This comment has been minimized.

Copy link
Author

JosejeSinohui commented Dec 17, 2019

@boneskull any follow up on this?

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 18, 2019

just that those don’t support BigInt but I’m confused about why they weren’t failing CI. could you look in to it? I don’t have time atm. hoping to have more time for project after new year

@JosejeSinohui

This comment has been minimized.

Copy link
Author

JosejeSinohui commented Dec 19, 2019

From what i see, the CI runs tests only on different NodeJs versions, Node 8 doesn't support BigInts either but I only added a new case to the stringify function, in that case it uses the toString function to create the representation of the BigInt, so the code it's never instantiating a BigInteger, just knows how to handle one if it ever receives it.
The build was failing before because in the test that I added was creating a BigInt to test that it was correctly parsed into a string with the stringify function. Because of this the test didn't pass in node8 because BigInteger wasn't supported there. To fix it I just added a check for support before trying it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.