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(format): correctly apply trailing comma to formatting #1944

Merged
merged 9 commits into from
Mar 1, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Feb 29, 2024

Summary

Closes #1659
Closes #1662

What started as a simple fix became a refactor that was needed anyway, so the sooner, the better.

The refactor was required due to a limitation we had in our WorkspaceServer. Every time we save a Document in our workspace, we store it together with an information called language_hint. This is used to tell the workspace the kind of language that belongs to a file.

However, there are information that we aren't able to compute from the file extension. For example, a .js file can be a script or a module, and this info, still today, can't be correctly computed. Plus, we couldn't tell the formatter that trailing commas are allowed, because this option is saved in the parsing options. These parsing options aren't available when computing the formatting options.

This refactor removes the Language enum and replaces it with a new enum called DocumentFileSource. The DocumentFileSource exposes the same functions that Language does, but it holds the information of a file source: e.g. JsFileSource, JsonFileSource, etc.

Initially, DocumentFileSource is inferred from the file extension so that logic isn't changed. Although we now pass this type around, we are also able to mutate it during the parsing phase.

For example, check the json.rs file in the parse() function. There, we mutate the JsonFileSource and change the newly created allow_trailing_comma.

In order to reduce memory usage, I created a new field called file_sources, which is an IndexSet, where we save all the known file sources that we find. Inside the Document struct we save the index of the newly created file source.

The idea is the following: if we analyze 100 .js files, it's safe to assume that all of them have the same JsFileSource, so we have only one copy index file_source, and we always retrieve the same when we need it.

Test Plan

As you might have noticed, now the JSOC files are correctly formatted :)

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Feb 29, 2024
Copy link

netlify bot commented Feb 29, 2024

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit 8a298e0
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65e1b7de9076f80008e3eae3

@ematipico ematipico requested review from a team February 29, 2024 14:55
@github-actions github-actions bot added A-Website Area: website A-Changelog Area: changelog labels Feb 29, 2024
Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 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 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 593 593 0
Failed 69 69 0
Panics 0 0 0
Coverage 89.58% 89.58% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13439 13439 0
Failed 4205 4205 0
Panics 2 2 0
Coverage 76.16% 76.16% 0.00%

Copy link

codspeed-hq bot commented Feb 29, 2024

CodSpeed Performance Report

Merging #1944 will not alter performance

⚠️ No base runs were found

Falling back to comparing fix/formatter-trailing-separator (8a298e0) with main (b0742b5)

Summary

✅ 93 untouched benchmarks

@Sec-ant
Copy link
Member

Sec-ant commented Feb 29, 2024

Hey @ematipico thank you very much for your work!

Still, I have a small question. Do you think it is reasonable to provide a dedicated trailing_commas_control option for the json formatter as well? So that we can use a forgiving option in the parser (allow trailing commas) but a stricter option in the formatter (omit trailing commas).

This is most useful when formatting tsconfig.json files, because although it can include trailing commas, 1) it has the extension .json so some other tools may see it as a standard json file, 2) VSCode issues warnings for jsonc files with trailing commas unless the user manually override the settings, 3) there has been a heated debate about this on the Prettier side: prettier/prettier#15956, Prettier first treated tsconfig.json as a jsonc file in v3.2.3 and would add trailing commas to the file, and later in v3.2.5 it reverted the change.

@ematipico
Copy link
Member Author

I believe it's a reasonable request! I think I will apply this change in another PR :)

@ematipico ematipico added this pull request to the merge queue Mar 1, 2024
Merged via the queue into main with commit 4c92db2 Mar 1, 2024
19 of 22 checks passed
@ematipico ematipico deleted the fix/formatter-trailing-separator branch March 1, 2024 11:12
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-LSP Area: language server protocol A-Parser Area: parser A-Project Area: project A-Website Area: website L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
2 participants