-
-
Notifications
You must be signed in to change notification settings - Fork 495
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: style recommendation and new recommended severity #4730
Conversation
CodSpeed Performance ReportMerging #4730 will not alter performanceComparing Summary
|
I like this change! I suggest using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn’t have time for a full review, but I’m on board too 👍
Just two suggestions:
- I think
biome migrate
should keep them enabled for users coming from Biome 1.x. For new users we can indeed leave them disabled by default going forward. - For
noVar
specifically:var
actually has confusing semantics, and can cause bugs in combination with loops, for instance. It feels this more than just a style choice, so maybe we need to recategorize the rule?
Definitely wise to do that. Maybe we can move it to the
That's wise, I will follow up with another PR |
Summary
Part of #4662
style
rules aren't recommended anymoreIt's possible that people won't agree with this choice, so let's discuss it. However, our recommended rules are a lot, and we should slim them down.
With the introduction of domains, there's also a faster and better (granular) way to add recommended rules.
Styling rules can be very opinionated, and we should avoid imposing a tool's opinion on users unless there's a design behind it. Our linter wasn't born to be opinionated by design, as opposed to our formatter. In the last months, I could see some displeasure among our users, asking why certain style rules were recommended, considering that until v1.*, the recommended ones were emitting an error.
Important
We can still revert the change, however I would like reduce the number of errors and noice. For example,
noVar
shouldn't be an error.New
"on"
optionUntil v1, when users wanted to enable non-recommended rules, they needed to choose a severity from
info
,warn
, anderror
.However, now that all rules have a default severity, users can decide to use the severity suggested by Biome. In the following example, when a user uses
"on"
fornoArguments
, violations of this rule will raise errors, because that's its default severity suggested by us. Instead,noNamespace
will raise an info diagnostics:Better
biome explain <RULE>
I didn't like the explanation of our rules. I revamped it, here's a preview from my terminal:
Test Plan
Added new tests and update many of our current tests