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

refactor(linter): severity and recommendation of lint rules #5133

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

ematipico
Copy link
Member

Summary

  • make the existing recommended set of rules less pedantic (the changeset presents a summary of it);
  • noForEach isn't recommended anymore because performance is debatable (it depends by the runtime) and it changes with time;
  • all the *useless* rules emit an information diagnostic, because they trigger ... unless code! no need to block the CI for it;
  • noDelete isn't recommended anymore because it's the way to for environment variables in Node.js, so Biome should be less pedantic about it;

Test Plan

Many snapshot tests are updated because our test infra was hardcoding the severity of our rules. Now that rules have their own severity, there's no need for that anymore.

@ematipico ematipico requested review from a team February 16, 2025 16:09
@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Feb 16, 2025
@ematipico ematipico force-pushed the refactor/default-severity branch from 670550d to 627948f Compare February 16, 2025 16:12
Copy link

codspeed-hq bot commented Feb 16, 2025

CodSpeed Performance Report

Merging #5133 will not alter performance

Comparing refactor/default-severity (33ff868) with main (d739883)

Summary

✅ 94 untouched benchmarks

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@ematipico ematipico marked this pull request as draft February 17, 2025 09:36
The following rules aren't recommended anymore:
- `noDelete`
- `noForEach`
- `noFlatMapIdentity`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this rule is no longer recommended?

Copy link
Member Author

@ematipico ematipico Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy has this rule under the pedantic category, and it doesn't recommended it, I think we should follow their lead and stick with it.

Our recommendation set is very opinionated and we should reduce it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many users like the fact that Biome is opinionated.
I accept that we could be more cautious about what we add in the recommended rule set.
Personally I could keep this rule in the recommended set because it brings one of the mission of Biome: making dev better dev by helping to learn JS API.
We could reduce the severity level to information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@ematipico ematipico marked this pull request as ready for review February 17, 2025 10:05
@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project labels Feb 17, 2025
@ematipico ematipico merged commit 84e0407 into main Feb 19, 2025
13 checks passed
@ematipico ematipico deleted the refactor/default-severity branch February 19, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants