feat(rome_json_parser): support allowTrailingCommas option #326

nissy-dev · 2023-09-18T10:19:50Z

Summary

Fix #129

Test Plan

I add and update some snapshot tests

crates/biome_service/src/file_handlers/json.rs

denbezrukov · 2023-09-18T10:31:01Z

Do we need to tweak the formatter's behavior?

nissy-dev · 2023-09-18T12:13:44Z

Do we need to tweak the formatter's behavior?

At this point, I think we do not need to do anything in particular. If we get feedback from users, we will consider it then.

CHANGELOG.md

ematipico

There is an important thing missing, we need to update our grammar to take this new syntax into account.

At the moment our grammar doesn't allow trailing commas, for example:

biome/xtask/codegen/json.ungram

Line 59 in 9bc8a92

JsonArrayElementList = (AnyJsonValue (',' AnyJsonValue)* )

This means that we have to update the grammar to take that into account. Here's an example of how we do it in JavaScript grammar:

biome/xtask/codegen/js.ungram

Line 424 in 9bc8a92

JsArrayElementList = (AnyJsArrayElement (',' AnyJsArrayElement)* ','?)

Look at the '.'? at the end.

ematipico · 2023-09-19T08:52:44Z

crates/biome_cli/tests/commands/format.rs

+    let config_json = r#"{
+    "json": {
+        "parser": { "allowTrailingCommas": true }
+    }
+}"#;
+    let biome_config = "biome.json";
    let code = r#"
-/*test*/ [
-
-/* some other comment*/1, 2, 3]
-	"#;
+/*test*/{
+    /* some other comment */
+    "array": [1, 2, 3],
+}
+"#;


I think we should create a new test, so we don't overlap with other tests

ematipico · 2023-09-19T08:56:23Z

crates/biome_json_parser/tests/json_test_suite/allow_trailing_commas/basic.json.snap

+                            value_token: JSON_NUMBER_LITERAL@27..37 "2" [Newline("\n"), Whitespace("        ")] [],
+                        },
+                        COMMA@37..38 "," [] [],
+                        missing element,


I believe there's something wrong with the AST here. This is because our grammar hasn't been udpated, and the syntax factory hasn't generated the code to take , into account.

A new token means that the syntax factory must generate a new syntax where we have a new slot where the token , is missing and optional.

github-actions · 2023-09-19T15:29:16Z

Parser conformance results on

js/262

Test result	`main` count	This PR count	Difference
Total	48863	48863	0
Passed	47808	47808	0
Failed	1055	1055	0
Panics	0	0	0
Coverage	97.84%	97.84%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	40	40	0
Passed	37	37	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.50%	92.50%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	6212	6212	0
Passed	1767	1767	0
Failed	4445	4445	0
Panics	0	0	0
Coverage	28.44%	28.44%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	639	639	0
Passed	571	571	0
Failed	68	68	0
Panics	0	0	0
Coverage	89.36%	89.36%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	17224	17224	0
Passed	13123	13123	0
Failed	4101	4101	0
Panics	0	0	0
Coverage	76.19%	76.19%	0.00%

nissy-dev · 2023-09-20T00:48:29Z

Thank you for reviews! I updated the codes by your reviews.

ematipico

Looking good, I think we should add more cases to avoid possible regressions or "unknown" in the future.

ematipico · 2023-09-20T08:18:52Z

crates/biome_json_parser/tests/json_test_suite/allow_trailing_commas/basic.json

+{
+    "prop1": [
+        1,
+        2,
+    ],
+    "prop2": 0,


We should add more cases, so we cover all the possible valid/not valid combinations:

true,

Valid or not? We should have a case for that

null,

Valid or not? We should have a case for that

{},

Valid or not? We should have a case for that

ematipico · 2023-09-20T13:47:14Z

crates/biome_json_parser/tests/json_test_suite/allow_trailing_commas/basic.json

Sorry, I actually meant having a json file that contains

true

(same for the rests)

They all are valid JSON files 😄

ematipico

I believe these new cases should fail, although you should check what VSCode does. If VSCode does support trialing comma in these cases, then we need:

to update the grammar
correctly parse these tokens before EOF

For example, at the moment there's an issue:

0: JSON_ROOT@0..6
  0: JSON_NULL_VALUE@0..4
    0: NULL_KW@0..4 "null" [] []
  1: EOF@4..6 ",\n" [] []

The comma is inside the EOF token

nissy-dev · 2023-09-22T12:55:01Z

Sorry, I didn't check properly. After checking jsonc-parser and VSCode, I confirm the new case need to fail. Therefore, I update codes and test cases.

ematipico

That's great! Looking forward to having this released 🚀

nissy-dev added 2 commits September 18, 2023 18:37

feat(rome_json_parser): support allowTrailingCommas options

cf21948

Merge branch 'main' into nissy-dev/json-parser-comma

0d61989

nissy-dev temporarily deployed to Website deployment September 18, 2023 10:19 — with GitHub Actions Inactive

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter A-Website Area: website L-JSON Language: JSON and super languages A-Changelog Area: changelog labels Sep 18, 2023

nissy-dev changed the title ~~feat(rome_json_parser): support allowTrailingCommas options~~ feat(rome_json_parser): support allowTrailingCommas option Sep 18, 2023

denbezrukov reviewed Sep 18, 2023

View reviewed changes

crates/biome_service/src/file_handlers/json.rs Outdated Show resolved Hide resolved

fix: update codes by reviews

a082e59

nissy-dev temporarily deployed to Website deployment September 18, 2023 12:12 — with GitHub Actions Inactive

fix: set false in jsonc

fcc9ca5

nissy-dev temporarily deployed to Website deployment September 18, 2023 12:49 — with GitHub Actions Inactive

fix: update snapshots

7a48440

nissy-dev temporarily deployed to Website deployment September 18, 2023 14:18 — with GitHub Actions Inactive

github-actions bot removed the A-Formatter Area: formatter label Sep 18, 2023

ematipico reviewed Sep 18, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

ematipico requested changes Sep 19, 2023

View reviewed changes

nissy-dev added 2 commits September 20, 2023 00:13

fix: update ungram

c2a22a8

fix: add test

a2a2d7b

nissy-dev temporarily deployed to Website deployment September 19, 2023 15:22 — with GitHub Actions Inactive

github-actions bot added the A-Tooling Area: internal tools label Sep 19, 2023

ematipico approved these changes Sep 20, 2023

View reviewed changes

test: update

97a1cd7

nissy-dev temporarily deployed to Website deployment September 20, 2023 13:30 — with GitHub Actions Inactive

ematipico reviewed Sep 20, 2023

View reviewed changes

test: add some case

63088c3

nissy-dev temporarily deployed to Website deployment September 20, 2023 15:29 — with GitHub Actions Inactive

ematipico reviewed Sep 20, 2023

View reviewed changes

fix: update test case by review

f6a7f20

nissy-dev temporarily deployed to Website deployment September 22, 2023 12:50 — with GitHub Actions Inactive

ematipico approved these changes Sep 22, 2023

View reviewed changes

nissy-dev merged commit 04de818 into main Sep 22, 2023
18 checks passed

nissy-dev deleted the nissy-dev/json-parser-comma branch September 22, 2023 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_json_parser): support allowTrailingCommas option #326

feat(rome_json_parser): support allowTrailingCommas option #326

nissy-dev commented Sep 18, 2023 •

edited

Loading

denbezrukov commented Sep 18, 2023

nissy-dev commented Sep 18, 2023

ematipico left a comment •

edited

Loading

ematipico Sep 19, 2023

ematipico Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading

nissy-dev commented Sep 20, 2023

ematipico left a comment

ematipico Sep 20, 2023

ematipico Sep 20, 2023

ematipico left a comment •

edited

Loading

nissy-dev commented Sep 22, 2023 •

edited

Loading

ematipico left a comment

feat(rome_json_parser): support allowTrailingCommas option #326

feat(rome_json_parser): support allowTrailingCommas option #326

Conversation

nissy-dev commented Sep 18, 2023 • edited Loading

Summary

Test Plan

denbezrukov commented Sep 18, 2023

nissy-dev commented Sep 18, 2023

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

ematipico Sep 19, 2023

Choose a reason for hiding this comment

ematipico Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023 • edited Loading

Parser conformance results on

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

nissy-dev commented Sep 20, 2023

ematipico left a comment

Choose a reason for hiding this comment

ematipico Sep 20, 2023

Choose a reason for hiding this comment

ematipico Sep 20, 2023

Choose a reason for hiding this comment

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

nissy-dev commented Sep 22, 2023 • edited Loading

ematipico left a comment

Choose a reason for hiding this comment

nissy-dev commented Sep 18, 2023 •

edited

Loading

ematipico left a comment •

edited

Loading

ematipico Sep 19, 2023 •

edited

Loading

github-actions bot commented Sep 19, 2023 •

edited

Loading

ematipico left a comment •

edited

Loading

nissy-dev commented Sep 22, 2023 •

edited

Loading