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(configuration): deprecate trailingComma in favor of trailingCommas #2492

Merged
merged 1 commit into from May 14, 2024

Conversation

Sec-ant
Copy link
Contributor

@Sec-ant Sec-ant commented Apr 17, 2024

Summary

Attempt to address this #1948 (comment).

  • I updated all the tests to use the new option name (plural form trailingCommas) instead of the old one, so there're many snapshot updates.
  • The old name is kept for backward compatibility. Fully removal requires a major version bump.
  • I found there're some bugs in properly handling the option indentSize. For example, when --indent-size and --indent-width are both provided, the code lacks the logic to prioritize --indent-width over --indent-size. I also fixed them in this PR.
  • I created a helper function for the JSON analyzer to conveniently match a JSON member name node against a path. For example, I can use it to test whether a "trailingComma" node matches the path ["javascript", "formatter", "trailingComma"]. I also updated the indentSize migration rule to use this function.
  • Some tests still use the option name indentSize, I updated them to use indentWidth.
  • I removed the Indent size row from the rage output.
  • Overall, the deprecation logic follows what we did for indentSize.

Test Plan

I added test cases to ensure the old name is still functional in CLI mode, and both of the old and new names are valid configurations.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Apr 17, 2024
Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 6b2b104
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/661f96a8721c940008c920cb
😎 Deploy Preview https://deploy-preview-2492--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🟢 up 4 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.

Copy link

codspeed-hq bot commented Apr 17, 2024

CodSpeed Performance Report

Merging #2492 will not alter performance

Comparing Sec-ant:fix/use-trailing-commas (eb4cfc2) with main (99d5f63)

Summary

✅ 97 untouched benchmarks

@Sec-ant Sec-ant changed the title fix(configuration): deprecate javascript.trailingComma in favor of javascript.traiilingCommas fix(configuration): deprecate javascript.trailingComma in favor of javascript.trailingCommas Apr 18, 2024
@Sec-ant Sec-ant closed this Apr 18, 2024
@Sec-ant Sec-ant reopened this Apr 18, 2024
@ematipico
Copy link
Member

ematipico commented Apr 24, 2024

@Sec-ant sorry, I missed that you wanted to know how to deprecate an option.

Overall it looks good. To create a warning, we usually do it via migration. Here's an example of formatter.indentSize: https://github.com/biomejs/biome/blob/main/crates/biome_migrate/src/analyzers/indent_size.rs

@Sec-ant
Copy link
Contributor Author

Sec-ant commented Apr 24, 2024

@Sec-ant sorry, I missed that you wanted to know how to deprecate an option.

Don't be sorry, I was planning to dig a little deeper, and thanks for the tips. But unfortunately I'm a little occupied these days. Still trying to figure out a way to better balance the time here and in my real life.

@Sec-ant Sec-ant force-pushed the fix/use-trailing-commas branch 2 times, most recently from 4f07426 to 2a6fd20 Compare May 2, 2024 17:03
@ematipico ematipico added the S-Feature Status: new feature to implement label May 3, 2024
@ematipico ematipico added this to the Biome 1.8 milestone May 6, 2024
@ematipico
Copy link
Member

What's left for this PR to land? Happy to help

@Sec-ant
Copy link
Contributor Author

Sec-ant commented May 12, 2024

What's left for this PR to land? Happy to help

TLDR, this PR now is not backward compatible, because I changed all the test cases to use trailingCommas instead of trailingComma, so all of them will pass. But this will be a breaking change for all the existing code.

When I first created this PR I was trying to find a solution to map both the trailingComma and trailingCommas inputs to a single representation in the internal structure. That's what I did for the bpaf command line args parsing (--trailing-comma is just an alias of --trailing-commas, and with the help of @pacak, I was also able to add some custom messages to indicate its deprecation). But I encountered problems on parsing the configuration JSON in this way. I think that's because we have our own deserialization based on serde that emits some further diagnostics (?), and that's where I have the problem implementing the similar alias mechanism like what I did with the command line parsing.

From what you suggested on how we handled formatter.indentSize, I think I was doing it in a wrong way: It seems that we need to keep both trailingComma and trailingCommas in the internal structure. And since then I haven't found enough large block of time to debug it, sorry about that.

@ematipico
Copy link
Member

So, configuration wise (biome.json and CLI), we will create a new field/option, the one that you want to create.

Then, when we do the deserialization to WorkspaceSettings, we will map this new value to the one that we currently have.

Workspace settings is an internal type, this means that we can rename it without repercussions.

Sure, users might have the two options at the same time, and we could create an error/warning, but we can do it later.

@github-actions github-actions bot added A-Parser Area: parser L-JSON Language: JSON and super languages A-Changelog Area: changelog labels May 13, 2024
@Sec-ant Sec-ant marked this pull request as ready for review May 13, 2024 05:57
@Sec-ant Sec-ant requested a review from ematipico May 13, 2024 06:00
@Sec-ant Sec-ant changed the title fix(configuration): deprecate javascript.trailingComma in favor of javascript.trailingCommas fix(configuration): deprecate trailingComma in favor of trailingCommas May 13, 2024
@Sec-ant Sec-ant force-pushed the fix/use-trailing-commas branch 2 times, most recently from ab61546 to f82ad61 Compare May 13, 2024 14:18
@Sec-ant Sec-ant merged commit 8aeaeb9 into biomejs:main May 14, 2024
12 checks passed
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-Linter Area: linter A-Parser Area: parser A-Project Area: project L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages S-Feature Status: new feature to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants