Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

fix: loose strategy when suffer trailing comma in json - #4379

Draft
IWANABETHATGUY wants to merge 2 commits into
rome:mainfrom
IWANABETHATGUY:fix/release-json-with-trailing-comma
Draft

IWANABETHATGUY wants to merge 2 commits into
rome:mainfrom
IWANABETHATGUY:fix/release-json-with-trailing-comma

Conversation

@IWANABETHATGUY

@IWANABETHATGUY IWANABETHATGUY commented Apr 16, 2023 •

Copy link
Copy Markdown
Contributor

Summary

{
"test", "something",
"another", "something",
}

trailing comma syntax error is a common syntax error when copying and pasting something in the json file, we could lose this kind of syntax error, and give our formatter a chance to format the code, like pretty did.

# Prettier differences
```diff
--- Prettier
+++ Rome
@@ -1 +1,9 @@
-[[1, null], [1, null], [null], [0], [false], [""]]
+[
+ [
+1,null],
+ [1,null,],
+ [null,],
+ [0,],
+ [false,],
+ ['',]
+]
```

Test Plan

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify

netlify Bot commented Apr 16, 2023 •

Copy link
Copy Markdown

✅ Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 9bc3f89
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/643bd352660dc50008c8f4f3

@github-actions github-actions Bot added the A-Parser Area: parser label Apr 16, 2023
@ematipico

Copy link
Copy Markdown
Contributor

I am a bit hesitant about this "fix":

  • we're not fixing anything, we're actually injecting non-spec changes to a language;
  • we can't compare ourselves to prettier in that sense. Rome does work in LSP too, and there might be people that rely on diagnostics even when the formatter is disabled;
  • if a JSON file is used by users inside an application and they don't use the formatter, that file will raise an error;

How did you want to introduce this change?

This branch has not been deployed

No deployments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-Parser Area: parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants