-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
BigInt & numeric separator support #204
Conversation
{ | ||
public readonly string BigInt; | ||
|
||
public BigInteger? BigIntValue => (BigInteger?) Value; |
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 we really need to add this as Literal already has that, I think it should be quite cheap to call even with the type check
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.
I did it to match the Tree spec: https://github.com/estree/estree/blob/master/es2020.md#bigintliteral
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.
Cause I think the AST should be nearly the same in all Javascript Parsers
src/Esprima/Scanner.cs
Outdated
if (Source.CharCodeAt(Index) == '_') | ||
ThrowUnexpectedToken(Messages.NumericSeperatorNotAllowedHere); | ||
|
||
while (!Eof() && (check(Source.CharCodeAt(Index)) || Source.CharCodeAt(Index) == '_')) { |
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.
in c# version we always add braces and they should on new line, I think that .editorconfig should enforce these?
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.
maybe an issue with vs2022?
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.
might be, just a thing to keep in mind so code is kept consistent
is there anything I need to do here? |
I guess there's no Esprima PR to diff this against? |
yes, I'll look when I've time to port my changes from esprima-next to esprima (should not be much work). but the development on esprima has nearly come to an halt, yes there were a few merges, but no reaction to questions, many dead branches... and no release since years... |
pull req in esprima: jquery/esprima#2105 |
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 to go, thanks for making the effort to get changes back to original JS project.
there is not yet a pull request for esprima