feat(formatter): add option `json.formatter.trailingComma` #1948

ematipico · 2024-03-01T13:38:28Z

Summary

This is a follow up of #1944

Closes #926

This PR adds the option json.formatter.trailingComma. Its options are the same of js.formatter.trailingComma, exception for es5, which doesn't have anything to do with JSON.

Warning

This option changes its value if json.parser.allowTrailingComma is set to true. That was done to have both parser and formatter to behave in the same way.

Let me know what you think about this decision ⬆️ Logically, I thought it made sense, because if a user wants to parse JSON trailing commas, and it makes sense to leave them there.

Test Plan

I added a new test case

netlify · 2024-03-01T13:38:43Z

✅ Deploy Preview for biomejs ready!

Name	Link
🔨 Latest commit	`b36a682`
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/65e2088aba4e4d0008837bef
😎 Deploy Preview	https://deploy-preview-1948--biomejs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 95 (🟢 up 1 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 93 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

ematipico · 2024-03-01T13:39:09Z

@Sec-ant I'd like your input here. What do you think?

codspeed-hq · 2024-03-01T14:06:35Z

CodSpeed Performance Report

Merging #1948 will degrade performances by 7.75%

_{⚠️ No base runs were found}

_{Falling back to comparing feat/trailing-comma-option (b36a682) with main (4c92db2)}

Summary

❌ 1 (👁 1) regressions
✅ 92 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`feat/trailing-comma-option`	Change
👁	`db.json[cached]`	70.4 ms	76.3 ms	-7.75%

arendjr · 2024-03-01T14:15:31Z

When this option isn't provided, and you set the parser option json.parser.allowTrailingComma to true, the value of json.formatter.trailingComma is "all".

For the VS Code extension at least, I would suggest the default value for json.parser.allowTrailingComma should be true, while the default value for json.formatter.trailingComma should be none. This makes the parser more lenient, while keeping the formatter more strict, which means we increase compatibility both with content loaded/pasted by the user, while also increasing compatibility on save with other tools that might be reading the JSON.

I'm less opinionated about the defaults for the CLI, although I guess it makes more sense if they'd be the same?

Sec-ant · 2024-03-01T14:38:21Z

Hey, thanks for mentioning. Here are my two cents:

Regarding Naming Conventions:

It's essential to maintain a consistent naming style across our configuration options. Given that we use plural forms in options like javascript.formatter.{semicolons, arrowParentheses, quoteProperties}, and json.parser.{allowComments, allowTrailingCommas}, I recommend we align the naming accordingly. Therefore, javascript.formatter.trailingComma should be updated to javascript.formatter.trailingCommas, and similarly, we should name the new option json.formatter.trailingCommas for consistency.
Concerning the Configuration Options:

Initially, I considered three settings: all, none, and passthrough (or a shorter alternative). The passthrough option signifies that we leave the trailing commas unchanged, minimizing surprises and unexpected code differences when developers upgrade. However, since the javascript.formatter.trailingCommas option does not include a passthrough choice, it might be reasonable to limit the options to just all or none.

If we decide to support passthrough, it should be the default to be as least surprising as possible. If not, considering that using trailing commas in jsonc files is generally discouraged, setting the default to none seems appropriate.

Furthermore, I believe the default configuration for the jsonc language should be as follows to prevent issues mentioned here:
```
{
  "json": {
    "parser": {
      "allowComments": true,
      "allowTrailingCommas": true
    },
    "formatter": {
      "trailingCommas": "none"
    }
  }
}
```
On the Mechanism:

I'm uncertain whether we should modify the json.formatter.trailingCommas option based on the json.parser.allowTrailingCommas setting. My stance is that json.formatter.trailingCommas should only be active when json.parser.allowTrailingCommas is true. If set to false, this option should be ignored. My understanding is that if json.parser.allowTrailingCommas is false and encounters a trailing comma, it should be treated as invalid syntax, resulting in a bogus node. Consequently, the formatter would not detect a trailing comma in such scenarios, treating the bogus node as plain text. Therefore, altering the json.formatter.trailingCommas value seems unnecessary.

ematipico · 2024-03-02T06:23:29Z

I applied all the suggestions.

A couple of notes:

@arendjr: a disparity between VSCode and CLI isn't possible at the moment. I mean, we can totally support that, by adding an option coming from the client (VSCode). Still, I have some reservations about the difference between editor and CLI. We can revisit this later
@Sec-ant: A passthrough option is definitely something we can evaluate. rustfmt does something similar. However, this is a bigger discussion that involves JavaScript too, since they both share the same mechanism

Sec-ant

@ematipico I left a few reviews which I hope wouldn't bother you too much.

And a last question is shouldn't we also update the name of javascript.formatter.trailingComma to javascript.formatter.trailingCommas? This is certainly a breaking change but now that we decide to use plural forms for every other option, I assume you agree with me right?

Sec-ant · 2024-03-02T06:50:39Z

CHANGELOG.md

@@ -619,6 +619,10 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

 ### Formatter

+#### New features
+
+- Add option `json.formatter.trailingComma`, to provide a better control over the trailing comma in JSON/JSONC files. Its default value is `"none"`.


json.formatter.trailingCommas

Sec-ant · 2024-03-02T06:52:11Z

crates/biome_cli/tests/commands/format.rs

+}
+
+#[test]
+fn format_omits_json_trailing_comma_omit() {


I know this is only for testing, but the function name seems a little confusing?

Sec-ant · 2024-03-02T06:56:57Z

crates/biome_service/src/configuration/json.rs

+    /// Print trailing commas wherever possible in multi-line comma-separated syntactic structures. Defaults to "omit".
+    #[partial(bpaf(
+        long("json-formatter-trailing-commas"),
+        argument("omit|allow"),


Shouldn't we use none and all or is there something I missed with the omit and allow choices?

Sec-ant · 2024-03-02T06:57:41Z

website/src/content/docs/reference/configuration.mdx

@@ -694,6 +694,16 @@ How many characters can be written on a single line in JSON (and its super langu

 > Default: `80`

+### `json.formatter.trailingComma`


json.formatter.trailingCommas

ematipico · 2024-03-02T07:21:51Z

Thank you for the review @Sec-ant , I will address them later.

I won't change existing options because they are breaking changes. We can create new options and deprecate the old ones, and eventually remove everything in V2.0

feat(formatter): add option json.formatter.trailingComma

49217d7

ematipico requested review from a team March 1, 2024 13:38

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter A-Website Area: website L-JSON Language: JSON and super languages A-Changelog Area: changelog labels Mar 1, 2024

fmt

8975454

fix regression

b21059c

apply suggestions

2cd670e

ematipico force-pushed the feat/trailing-comma-option branch from 32f5328 to 2cd670e Compare March 1, 2024 16:10

github-actions bot added the A-LSP Area: language server protocol label Mar 1, 2024

ematipico added 2 commits March 1, 2024 16:46

codegen

f0a9297

internal codegen apply

b36a682

ematipico enabled auto-merge March 2, 2024 06:24

ematipico disabled auto-merge March 2, 2024 06:28

ematipico enabled auto-merge March 2, 2024 06:29

ematipico merged commit d945159 into main Mar 2, 2024
22 checks passed

ematipico deleted the feat/trailing-comma-option branch March 2, 2024 06:31

Sec-ant reviewed Mar 2, 2024

View reviewed changes

Sec-ant mentioned this pull request Apr 17, 2024

fix(configuration): deprecate trailingComma in favor of trailingCommas #2492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(formatter): add option `json.formatter.trailingComma` #1948

feat(formatter): add option `json.formatter.trailingComma` #1948

ematipico commented Mar 1, 2024 •

edited

Loading

netlify bot commented Mar 1, 2024 •

edited

Loading

ematipico commented Mar 1, 2024

codspeed-hq bot commented Mar 1, 2024 •

edited

Loading

arendjr commented Mar 1, 2024

Sec-ant commented Mar 1, 2024

ematipico commented Mar 2, 2024 •

edited

Loading

Sec-ant left a comment

Sec-ant Mar 2, 2024

Sec-ant Mar 2, 2024

Sec-ant Mar 2, 2024

Sec-ant Mar 2, 2024

ematipico commented Mar 2, 2024

		@@ -694,6 +694,16 @@ How many characters can be written on a single line in JSON (and its super langu

		> Default: `80`

		### `json.formatter.trailingComma`

feat(formatter): add option json.formatter.trailingComma #1948

feat(formatter): add option json.formatter.trailingComma #1948

Conversation

ematipico commented Mar 1, 2024 • edited Loading

Summary

Test Plan

netlify bot commented Mar 1, 2024 • edited Loading

✅ Deploy Preview for biomejs ready!

ematipico commented Mar 1, 2024

codspeed-hq bot commented Mar 1, 2024 • edited Loading

CodSpeed Performance Report

Merging #1948 will degrade performances by 7.75%

Summary

Benchmarks breakdown

arendjr commented Mar 1, 2024

Sec-ant commented Mar 1, 2024

ematipico commented Mar 2, 2024 • edited Loading

Sec-ant left a comment

Choose a reason for hiding this comment

Sec-ant Mar 2, 2024

Choose a reason for hiding this comment

Sec-ant Mar 2, 2024

Choose a reason for hiding this comment

Sec-ant Mar 2, 2024

Choose a reason for hiding this comment

Sec-ant Mar 2, 2024

Choose a reason for hiding this comment

ematipico commented Mar 2, 2024

feat(formatter): add option `json.formatter.trailingComma` #1948

feat(formatter): add option `json.formatter.trailingComma` #1948

ematipico commented Mar 1, 2024 •

edited

Loading

netlify bot commented Mar 1, 2024 •

edited

Loading

codspeed-hq bot commented Mar 1, 2024 •

edited

Loading

ematipico commented Mar 2, 2024 •

edited

Loading