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

refactor(parse/json): change fields in JsonParserSettings to Option #3272

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Jun 24, 2024

Summary

This changes the JSON parser options to use Option<bool> for better semantics when dealing with default values and fallbacks.

as discussed here: #3260 (comment)

Test Plan

CI should pass

Copy link

codspeed-hq bot commented Jun 24, 2024

CodSpeed Performance Report

Merging #3272 will improve performances by 6.73%

Comparing dyc3:06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ (a2574da) with main (a3c2b18)

Summary

⚡ 1 improvements
✅ 89 untouched benchmarks

Benchmarks breakdown

Benchmark main dyc3:06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ Change
jquery.min.js[cached] 28.7 ms 26.9 ms +6.73%

@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from adf5158 to 12c6730 Compare June 24, 2024 12:13
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from 12c6730 to 5d1aab5 Compare June 24, 2024 13:09
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch from 5d1aab5 to a2574da Compare June 24, 2024 18:44
@dyc3 dyc3 requested a review from Sec-ant June 24, 2024 23:54
Copy link
Member

@Sec-ant Sec-ant left a comment

Choose a reason for hiding this comment

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

Great fix! Could you also remove JsonParserOptions from the PR title?

@dyc3 dyc3 changed the title refactor(parse/json): change fields in JsonParserOptions and JsonParserSettings to Option refactor(parse/json): change fields in JsonParserSettings to Option Jun 25, 2024
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 25, 2024

Done

@Sec-ant Sec-ant merged commit 91522a5 into biomejs:main Jun 25, 2024
13 checks passed
@dyc3 dyc3 deleted the 06-23-refactor_parse_json_change_fields_in_jsonparseroptions_and_jsonparsersettings_to_option_ branch June 26, 2024 13:20
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Parser Area: parser A-Project Area: project L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants