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(overrides): json.formatter.trailingCommas should work #2025

Merged

Conversation

Sec-ant
Copy link
Member

@Sec-ant Sec-ant commented Mar 9, 2024

Summary

  • Respect json.formatter.trailingCommas option in overrides
  • Rename internal trailing_comma-named json-related vars to trailing_commas for consistency

Fixes #2009

Test Plan

Several tests are added in crates/biome_cli/tests/commands/format.rs to show that json.formatter.trailingCommas works in overrides.

- Respect `json.formatter.trailingCommas` option in `overrides`
- Rename internal `trailing_comma`-named json-related vars to `trailing_commas` for consistency
- Fix several discrepancies with reference to how `javascript.formatter.trailingComma` is handled
@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter L-JSON Language: JSON and super languages labels Mar 9, 2024
Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit 934fc12
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65ecb5bb8eb6840008c311aa

Copy link

codspeed-hq bot commented Mar 9, 2024

CodSpeed Performance Report

Merging #2025 will degrade performances by 6.09%

⚠️ No base runs were found

Falling back to comparing Sec-ant:fix-json-formatter-trailing-commas-in-overrides (934fc12) with main (7ae4529)

Summary

❌ 1 regressions
✅ 92 untouched benchmarks

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

Benchmarks breakdown

Benchmark main Sec-ant:fix-json-formatter-trailing-commas-in-overrides Change
eucjp.json[cached] 4.2 ms 4.5 ms -6.09%

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.

That's a great fix! 🥳 I just left a comment for consistency

@@ -71,7 +71,7 @@ pub struct JsonFormatter {

/// Print trailing commas wherever possible in multi-line comma-separated syntactic structures. Defaults to "none".
#[partial(bpaf(long("json-formatter-trailing-commas"), argument("none|all"), optional))]
pub trailing_commas: Option<TrailingCommas>,
Copy link
Member

Choose a reason for hiding this comment

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

I would like to keep this consistent with the rest of the CLI options.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted them in c0d3ab2.

There're some failures in CI tests, I'll take a look at them tomorrow.

@ematipico ematipico merged commit 75eed2e into biomejs:main Mar 10, 2024
16 of 17 checks passed
@Sec-ant Sec-ant deleted the fix-json-formatter-trailing-commas-in-overrides branch March 10, 2024 01:41
@Conaclos Conaclos added the A-Changelog Area: changelog label Mar 11, 2024
@flazouh
Copy link

flazouh commented Sep 2, 2024

still doesn't work
Screenshot 2024-09-02 at 02 22 12

@Conaclos
Copy link
Member

Conaclos commented Sep 2, 2024

@flazouh I think your $schema field is outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Formatter Area: formatter A-Project Area: project L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📝 Can't override json.formatter.trailingCommas
4 participants