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 support for BigInt numeric #588

Merged
merged 1 commit into from Nov 21, 2019
Merged

Add support for BigInt numeric #588

merged 1 commit into from Nov 21, 2019

Conversation

@zcei
Copy link
Contributor

zcei commented Jul 26, 2018

Description of the Change

TC39 added BigInt (currently at stage 3) and it's already available in e.g. Node.
The grammer currently doesn't support it and yields an invalid.illegal.identifier.js.

You can see this for example in the README of tc39/proposal-bigint

Alternate Designs

The octal regex could be expanded to not allow the n for the legacy octal definition. As this would have complicated things, I left it out. Let me know whether you think it should be added.

Benefits

Proper GitHub syntax highlighting with BigInts.

Possible Drawbacks

Not really applicable as this willt part of the

Applicable Issues

n/a

Other

tree-sitter/tree-sitter-javascript#93

grammars/javascript.cson Outdated Show resolved Hide resolved
@zcei zcei force-pushed the zcei:grammar-bigint branch from 5f6917c to f17feaa Jul 29, 2018
@zcei

This comment has been minimized.

Copy link
Contributor Author

zcei commented Aug 2, 2018

@50Wliu can you maybe have another look whenever it fits? 🙏

@bnoordhuis

This comment has been minimized.

Copy link

bnoordhuis commented Dec 11, 2018

I don't want to be the bump guy but I just published a module today that uses bigints extensively and it shows up real ugly (angry red example). It would be great if this got merged. Thanks in advance!

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 29, 2019

Once we know which version of ECMAScript this proposed feature will land in, we'll take a closer look at it. Thanks for the submission!

@bnoordhuis

This comment has been minimized.

Copy link

bnoordhuis commented Jan 31, 2019

@lee-dohm It's at stage 3 in TC39 and it's shipping in at least one browser (Chrome, flag-gated in Firefox) and Node.js. That's good enough, isn't it? It's out there in the real world, it's not a hypothetical.

(It also seems to have been implemented in Chakracore but I don't know if it's actually been shipped in IE. I'm guessing 'no' what with the transition to a different engine.)

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jan 31, 2019

It's at stage 3 in TC39 and it's shipping in at least one browser (Chrome, flag-gated in Firefox) and Node.js. That's good enough, isn't it?

We're aware. We discussed it as a team and decided that we felt that knowing what version of ECMAScript it would appear in would be our bar for inclusion.

@rsese rsese mentioned this pull request Feb 20, 2019
1 of 1 task complete
@zcei

This comment has been minimized.

Copy link
Contributor Author

zcei commented Jun 5, 2019

Hej there 👋

TC39 June meeting just happened and BigInt advanced to stage 4 (see the checkmark next to the agenda item) and thus will be included in the next formal spec release (ECMAScript 2020).

The PR towards ecma262 describing BigInt can be found here: tc39/ecma262#1515

I understand you might want to wait until this PR has landed (one never knows what might happen), just wanted to let you know 🙂

@lkashef lkashef mentioned this pull request Nov 12, 2019
1 of 1 task complete
@lkashef lkashef self-assigned this Nov 12, 2019
@lkashef

This comment has been minimized.

Copy link
Contributor

lkashef commented Nov 19, 2019

Hi @zcei, thank you for your contribution 🙇‍♂️.

However, I am going to close this PR, as it seems that this PR on tree-sitter adds support for BigInt (tree-sitter/tree-sitter-javascript#93).

@lkashef lkashef closed this Nov 19, 2019
@michaelficarra

This comment has been minimized.

Copy link

michaelficarra commented Nov 19, 2019

@lkashef The javascript.cson changes are still needed for GitHub linguist. Compare master with this PR.

@zcei

This comment has been minimized.

Copy link
Contributor Author

zcei commented Nov 20, 2019

As long as GitHub doesn't (fully?) use tree-sitter, I'd still love to see the changes landing. At least e.g. the example from Ben above still shows up red on GitHub - this visual flaw was my main intention for the contribution.

@lkashef

This comment has been minimized.

Copy link
Contributor

lkashef commented Nov 20, 2019

Thanks @zcei for the context, this issue clearly exist on Github, while I was looking into Atom.

Thanks @michaelficarra for the references.

@lkashef lkashef reopened this Nov 20, 2019
@lkashef lkashef merged commit 0bbdc0e into atom:master Nov 21, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zcei zcei deleted the zcei:grammar-bigint branch Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.