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

Fix a parser bug where tokens are misidentified as commas. #1502

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

vslashg
Copy link
Contributor

@vslashg vslashg commented Jul 19, 2023

In the old and new readers, when parsing an object, a comment followed by any non-} token is treated as a comma.

The new unit test required changing the runjsontests.py flag regime so that failure tests could be run with default settings.

In the old and new readers, when parsing an object, a comment
followed by any non-`}` token is treated as a comma.

The new unit test required changing the runjsontests.py
flag regime so that failure tests could be run with default settings.
Much of the comment handling in the parsers is bespoke, and does not
honor this flag.  By unfiying it under a common API, the parser is
simplified and strict mode is now more correctly strict.

Note that allowComments mode does not allow for comments in
arbitrary locations; they are allowed only in certain positions.
Rectifying this is a bigger effort, since collectComments mode requires
storing the comments somewhere, and it's not immediately clear
where in the DOM all such comments should live.
@vslashg
Copy link
Contributor Author

vslashg commented Jul 19, 2023

Sorry, I wasn't expecting my second change to get merged into this PR. I would still like both to be reviewed, but let me know if you'd like me to split this out somehow. (The second change relies on test framework changes made to the first.)

@baylesj
Copy link
Contributor

baylesj commented Sep 10, 2024

LGTM

@baylesj baylesj merged commit 0a9b9d9 into open-source-parsers:master Sep 10, 2024
6 of 7 checks passed
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.

2 participants