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(json_parser): fix broken JsonParserOptions::from(&JsonFileSource) implementation #4540

Merged

Conversation

cr7pt0gr4ph7
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 commented Nov 14, 2024

Summary

Fixes incorrect use of with_* methods in JsonParserOptions::from(&JsonFileSource). Adds unit tests and #[must_use] to prevent this error in the future.

I accidentally stumbled upon this while trying to fix some tests that were breaking due to an unrelated change. Took a bit of time to figure out that JsonParserOptions::from simply discarded the allow_comments and allow_trailing_commas flags...

Test Plan

Unit tests for JsonParserOptions::from(&JsonFileSource) were added.

@github-actions github-actions bot added A-Parser Area: parser L-JSON Language: JSON and super languages labels Nov 14, 2024
@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the fix-json-parser-options-from-file-source branch from fb373ea to 271bfbd Compare November 14, 2024 19:23
@cr7pt0gr4ph7 cr7pt0gr4ph7 changed the title fix(json_parser): Fix broken From<JsonFileSource> implementation for JsonParserOptions fix(json_parser): fix broken JsonParserOptions::from(&JsonFileSource) implementation Nov 14, 2024
@cr7pt0gr4ph7 cr7pt0gr4ph7 force-pushed the fix-json-parser-options-from-file-source branch from 271bfbd to e4302fc Compare November 14, 2024 20:06
@cr7pt0gr4ph7
Copy link
Contributor Author

cr7pt0gr4ph7 commented Nov 14, 2024

Format & test issues should be fixed now 👍

Copy link

codspeed-hq bot commented Nov 14, 2024

CodSpeed Performance Report

Merging #4540 will not alter performance

Comparing cr7pt0gr4ph7:fix-json-parser-options-from-file-source (e4302fc) with main (a3ffdd7)

Summary

✅ 99 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! We should start using must_use more often with the builder pattern.

I believe clippy provides a rule for that

@cr7pt0gr4ph7
Copy link
Contributor Author

The failure of the codegen build job seems to be caused by someone else having forgot to run the codegen for the jsonrpc bindings, and is not related to this PR.

@dyc3 dyc3 merged commit 927b75b into biomejs:main Nov 14, 2024
11 of 12 checks passed
@cr7pt0gr4ph7 cr7pt0gr4ph7 deleted the fix-json-parser-options-from-file-source branch November 14, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Parser Area: parser L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants