-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make the JavaScript parser compliant with JSON5 specs #43
base: master
Are you sure you want to change the base?
Conversation
a321bd2
to
df91e0d
Compare
3516389
to
3025f34
Compare
0bf7ae8
to
c8bc165
Compare
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've reviewed only a part for now, and I've added some more tests that must pass but are failing with this implementation, check them out.
1382d35
to
2a3c50e
Compare
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.
Some parts of the PR are not very clear to me, and parser is broken for different possible inputs I've provided. You should add tests for those inputs to ensure that they work correctly, before or after fixing the parser.
// Parse a number | ||
// | ||
parseNumber() { | ||
this.skipClutter(); |
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.
Each time this function is called clutter is already skipped. Because of this we can drop this line. Although we intend to have this as a part of a public API -- we should keep it. @belochub WDYT?
No description provided.