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.

Sign up for GitHub

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

Jump to bottom

refactor(formatter): no trailing commas in JSON files #4803

Merged
merged 2 commits into from
Jan 1, 2025
Merged

refactor(formatter): no trailing commas in JSON files #4803

merged 2 commits into from
Jan 1, 2025

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Dec 29, 2024 •
edited
Loading

Summary

Closes #4755

I added a new variant field to JsonFileSource. The value of this field is determined from the extension of the file, regardless of the known files and configuration options.

When JsonFileVariant is Standard, the formatter never adds trailing commas regardless of the configuration.

This is a breaking change.

Test Plan

Added new tests

@github-actions github-actions bot added A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages labels Dec 29, 2024
@ematipico ematipico requested review from a team December 29, 2024 12:37
@ematipico ematipico linked an issue Dec 29, 2024 that may be closed by this pull request
json.formatter.trailingCommas breaking package.json files #4755
Open
1 task
@ematipico ematipico force-pushed the refactor/json-trailing-comma-fmt branch from 8f97ec1 to 0709415 Compare December 29, 2024 12:41
@github-actions github-actions bot added the A-Tooling Area: internal tools label Dec 29, 2024
@ematipico ematipico force-pushed the refactor/json-trailing-comma-fmt branch from 0709415 to 57b0c2d Compare December 29, 2024 12:43
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Dec 29, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #4803 will degrade performances by 6.7%

Comparing refactor/json-trailing-comma-fmt (5ccf64e) with next (f342128)

Summary

❌ 1 regressions
✅ 96 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark next refactor/json-trailing-comma-fmt Change
❌ react.production.min_3378072959512366797.js[cached] 1.8 ms 2 ms -6.7%

@ematipico ematipico force-pushed the refactor/json-trailing-comma-fmt branch from 57b0c2d to 041ea98 Compare December 30, 2024 10:35
@github-actions github-actions bot added the A-CLI Area: CLI label Dec 30, 2024
arendjr
arendjr approved these changes Jan 1, 2025
View reviewed changes
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

The code looks good, but I do worry a bit about the user implications, since it leaves the option in quite a counter-intuitive place. At the least, we should document this very explicitly.

crates/biome_test_utils/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_json_syntax/src/file_source.rs Outdated Show resolved Hide resolved
@ematipico ematipico merged commit f86999d into next Jan 1, 2025
9 of 11 checks passed
@ematipico ematipico deleted the refactor/json-trailing-comma-fmt branch January 1, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arendjr arendjr arendjr approved these changes

Assignees
No one assigned
Labels
A-CLI Area: CLI A-Formatter Area: formatter A-Parser Area: parser A-Project Area: project A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

json.formatter.trailingCommas breaking package.json files
2 participants
@ematipico @arendjr