Replies: 12 comments 31 replies
-
|
I really like the idea. I have been advocating a flatter configuration and a tag system (here ruleSets) for some time :) Here some comments and suggestions: Rule setsI wonder if {
"linter": {
"ignore": ["./dist"],
"recommended": true,
"pedantic": false,
"style": true,
"noBlankTarget": "off",
}
}Just to be sure, a rule is enabled if at least one rule set to which it belongs is enabled (or if it is individually enabled), right? I wonder if we should not switch to levels instead of Boolean for the rule sets. This could allow setting a diagnostic level for an entire rule set: {
"linter": {
"ignore": ["./dist"],
"recommended": "error",
"pedantic": "warn",
"style": "off",
"noBlankTarget": "off",
}
}By the way, I think we should keep a base subset of disjoint rule sets (i.e. the actual categories), and add rule sets when it is useful (react, jest, ...). Note that we should not introduce a
|
Beta Was this translation helpful? Give feedback.
-
|
All right, by looking at what eslint is about to do, I believe it's a fair assessment to keep the Hence I propose this change: {
"linter": {
"rules": {
- "a11y": {
"noBlankTarget": "off"
- }
},
+ "presets": {
+ "a11y": "warn"
+ }
}
}While this isn't very flattened (only one level less), I believe it's a good compromise between verbosity and being concise.
It becomes too easy for a user to opt-in to nursery rules, so easy that it's difficult to understand when a rule is in active development or not; remember that nursery rules are meant for us and early adopters, not for beginners and active developers. So, a user needs to know when they are enabling a lint rule when they are suppressing it, and see it in the diagnostic category.
That's what nursery rules are meant for, with this proposal we restrain ourselves from doing all the changes we want as long as the nursery rules are in the incubating phase
Before being stabilized, rules can change name, behaviour... anything. It's an unfair assessment to assume that we are perfect developers and we can get a lint rule just right at the first PR. A first-time contributor should have all the time , and make all the PRs they want to make a nursery rule stable.
If with other tools you mean ESlint, I don't think it's a fair comparison. We aren't meant to be a drop-in replacement. Also clippy uses snake case for their rules, because snake case is the de facto standard of the Rust projects. Our rule names are consistent with the naming convention of our configuration file. Plus, most JavaScript code bases in the wild use I don't think this argument is strong enough to justify such a big change. |
Beta Was this translation helpful? Give feedback.
-
|
I think having both rule sets and project kinds is confusing. Why not combine the two concepts? |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, I thought so. My idea is that Ideally |
Beta Was this translation helpful? Give feedback.
-
|
What do you think about my take here? #689 (comment) |
Beta Was this translation helpful? Give feedback.
-
|
I have done some research in order to make a proposal for revamping linter configuration. State of the artClippyA Clippy lint rule emits diagnostics with a given severity level among
A user can set the severity level of a rule, a group, or the meta group
For example, let's assume we have the following configuration: [lints.clippy]
all = "warn"
pedantic = "warn"
empty_enum = "allow"And we use the following command ( cargo clippy -- -A clippy::all -D clippy::empty_enumTaking the CLI options into account, we obtain the following in-memory config: [lints.clippy]
all = "allow"
pedantic = "warn"
empty_enum = "deny"This disables all rules, then set all rules of Clippy provides also a dynamic group named Some observations and remarks:
Rustc linterRustc organizes rule into groups.
A user can set a severity level on an entire group. RuffRuff implements more than 700 rules from 50 distinct sources (plugins in Ruff terminology). Ruff allows selecting or ignoring a given code or any prefix of codes. Ruff also provides the code Similarly to Clippy, Ruff uses precedence rules to determine whether a rule is selected or ignored.
To select unstable rules, a user must turn on the Some observations and remarks:
TypeScript ESLintESLint rules have an associated severity level among ESLint provides plugins to add more linter rules. ESLint plugins provide presets of rules by relying on ESLint's configuration extension.
And 3 derived presets that include rules that require type checking:
It also provides the Some observations and remarks:
Biome (current state)Biome organizes its rules into exclusive groups.
Biome provides two presets: A rule is associated to a severity level among A CLI flag Some observations and remarks:
Proposal V1The Biome linter is more complex than any other reviewed linter, This proposal takes inspiration of both Clippy and Ruff. A rule is either stable or unstable (experimental / in a nursery state) and is either enabled or disabled. Every stable rule belongs to a unique group ( A rule is associated to a severity level among A rule belongs to one or several (most likely one) domains. A rule is enabled if and only if it is individually enabled or if its group and at least one of its domain are enabled. By default, a set of groups and domains are enabled. I think we should separate rules' options in a dedicated field. Let's take an example: {
"experimental": true, // make possible to individually enable an unstable rule
"linter": {
"enabled": true,
"groups": { // enable/disable groups
"correctness": true,
"idiomatic": true,
"style": false
},
"domains": { // enable/disable domains
"default": true,
"jest": true,
"react": false
},
"rules": { // Individually enables/disables rules
"useConsistentArraySyntax": true,
"useReactHooks": true,
"noSuperWithoutExtends": false,
"useAnUnstableRule": true // Setting this unstable rule causes an error if `experimental` isn't `true`
},
"options": { // rules' options
"useConsistentArraySyntax": {
"syntax": "shorthand"
},
"useNamingConvention": {
"enumMemberCase": "Pascal"
}
}
}
}In the preceding configuration, the user explicitly:
Some remarks and questions:
To bring some compatibility with the current configuration:
EDIT: taking feedbacks into account, I wrote a second version. |
Beta Was this translation helpful? Give feedback.
-
|
Some feedback. Overall I like the proposal, here's some thoughts:
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the feedback!
I am starting to lean towards being able to configure severity levels for groups. This makes sense as the severity of a rule depends on the group it belongs to. It also allows all linter diagnostics to be set to warnings (which a user requested). I am not sure to see the interest of setting the severity level for a given rule.
I think we can start without presets and add them later if we need to.
We can still use the proposal of @ematipico about |
Beta Was this translation helpful? Give feedback.
-
In the current proposal, options still need to be under a rule name. Thus, there is no option sharing between rules. I am still on the fence about rules' options. I think it needs more thought. This looks strange of repeating rule name in both The current state and the current proposal have some issues regarding cross-language rules with options. I remember that @ematipico suggested using identical names for rules of different languages with the same purpose. For example, we could have two rules internally called 'noBlankTarget': one for JSX and one for HTML. To the user, this looks like a single rule. However, this plays poorly with rules' options. For example, the options of the cross-language rules 'useNamingConvention' and 'useFilenamingConvention' could be language dependent. Because we have only a few rules with options (only 8 rules – about 4% of all rules), I wonder if we could merge all rules options into a single object. Some options could only be set on the concerned language (similarly to formatter options), and others could be set globally and overridden by languages. For example: {
"linter": {
"options": {
"strictCase": false
}
}
"javascript": {
"linter": {
"options": {
"strictCase": true, // override
"arrayTypeSyntax": "shothand",
"enumMemberCase": "PascalCase",
"fileNameCase": ["camelCase", "export"]
}
}
}
}This also allows sharing options between rules – for example Any opinion? |
Beta Was this translation helpful? Give feedback.
-
Maybe
If we want to provide the option, then we could accept a boolean or a severity level for turning rules. |
Beta Was this translation helpful? Give feedback.
-
|
I would be totally fine with a single unified From a DX perspective it is also a lot of hassle currently to add an options object to a rule, so that would be another advantage. And the JSON schema would become a lot simpler. I see only two possible downsides:
|
Beta Was this translation helpful? Give feedback.
-
Proposal V2This is the second version of the previous proposal, taking feedbacks into account and making it more compatibile with the current configuration system. Every rule belongs to a unique group ( NOTE: We could also introduce the supergroup (a set of groups) A rule is associated to a severity level among NOTE: I initially chose A rule belongs to one or several (most likely one) domains. A rule is enabled if and only if:
By default, a set of domains are enabled, and some groups have a default severity level set to NOTE: We should split the current For the time being, rule options are still set directly on the rule. Nursery rules are always prefixed by Let's take an example: {
"linter": {
"enabled": true,
"groups": {
"correctness": "error",
"idiomatic": "warn",
"style": "off",
"nursery": "warn" // Unstable rules
},
"domains": { // enable/disable domains
"base": true,
"jest": true,
"react": false
},
"rules": {
"useConsistentArraySyntax": {
"level": "warn",
"options": {
"syntax": "shorthand"
}
},
"useReactHooks": "group",
"noSuperWithoutExtends": "off",
"useNamingConvention": {
// default level: `group`
"options": {
"enumMemberCase": "Pascal"
}
},
// nursery rule must be prefixed with `nursery/`
"nursery/useAnUnstableRule": "error"
}
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for writing this down, looks quite good! Only thing I’m sad about is to see the rule options still being defined on the rules themselves, since I think that rules out sharing options between rules. It also makes it harder for rule developers wishing to add options to their rule (I remember adding the The reason I care particularly about sharing rule options is because fixers may need to respect settings from other rules as well. For instance, if we have a fixer that creates an array variable declaration, it should respect the setting from I think there is also a proposal to split the React |
Beta Was this translation helpful? Give feedback.
-
|
Shared options are a risky bet. While I understand and agree with your points @arendjr, sharing options introduces some concerning shortcomings:
Given my case, which route is more convenient to the user? Both have pros and cons |
Beta Was this translation helpful? Give feedback.
-
|
I would expect concerns 1 and 3 to be mostly resolved through documentation and explicit naming. If options have different semantics in different rules, they probably should be named differently. Looking at Conaclos' example above, the As for concern 2, I have trouble imagining an example where that would be problematic. Rather, I'd feel it's an advantage. You'd normally want to have the configuration be aligned across rules. Thinking of the example of a fixer that inserts an array again, you'd want it to respect the The latter point also makes me think shared options are indeed more convenient to the user. It's easier to maintain a configuration if you don't need to manually synchronize separate rules' options. The example of The main concern, whether options are aptly named and well documented, falls on us as developers (but we also gain other advantages). |
Beta Was this translation helpful? Give feedback.
-
I just came to the discussions to open a request to not have all rules default to Many editors / plugins of editors have features like "filter diagnostics by severity" or simply highlight diagnostics in different colors, and having everything that comes from biome being an error is restricting the usage of those features. |
Beta Was this translation helpful? Give feedback.
-
|
A separate idea I had, which might become relevant if we indeed end up including a JS engine in Biome for plugin support, is that a JS engine would also enable us to support import type { BiomeConfiguration } from "$biome-config";
import { config as base } from "../biome.config.ts";
export const config: BiomeConfiguration = {
...base,
formatter: {
...base.formatter,
indentWidth: 4
}
};It gives more flexibility to users, since they can very selectively choose which parts they want to inherit and which they don’t, and the semantics are unambiguous since they’re defined by JS. |
Beta Was this translation helpful? Give feedback.
-
|
I don't like the idea of introducing a JavaScript VM on each execution of Biome. It would become a bottleneck, and I think it's possible to come up with ways to provide a decent extension API with static JSON. The JS option is definitely the most flexible and I'm happy that eslint, for example, has taken this direction, but I don't think the trade-off in perf is worth it for Biome. Any potential challenges in extension/config merging behavior don't seem too hard to address in a JSON format. In the "worst" case, a minimal pseudo-DSL could work - a way to express what you want to merge/override (e.g. |
Beta Was this translation helpful? Give feedback.
-
|
To clarify, I do understand that we might need a JS VM for plugins, but I think they'd always be an opt-in thing anyway. |
Beta Was this translation helpful? Give feedback.
-
Just for the record, what I'm proposing for the configuration would be opt-in too. It would produce the exact same configuration object as the JSON files would, so plain JSON files would remain supported in exactly the same way. But if we have support for both, we can drop the
How much overhead do you expect a JS execution to introduce, and how much is too much? The config is only evaluated once per Biome execution, so if we keep it below, say, 10ms, would anyone care? What about 1ms? |
Beta Was this translation helpful? Give feedback.
-
|
I like the structure but I think there's still a lot of value in the prefixes/groups. I would propose that the rule names include it as part of the string, e.g. Tangentially related, but I think some of the framework-specific rules (like hook stuff) should be more explicitly grouped under an explicit "react" group. If there's an overlap with other frameworks (e.g. "preact"), it should always be possible to introduce the concept of "aliases", so that there could be something like Something like this might also prevent the need for adding a secondary "grouping" concept like domains and such. "Groups" could serve a double purpose in this way, making it simpler to understand for users, with the (imo very minor) trade-off of potential yet unlikely conflicts that can be very easily detected and communicated to users. |
Beta Was this translation helpful? Give feedback.
-
|
Looking again at the v2 proposal, it looks like it depends on the groups enabled. I understand the reasoning behind it but it still feels confusing to me. As a user, I'd want to do something like It feels too complicated. A flat concept of groups is easier to grasp: just presets ("recommended"/"all") that you can further customize by adding to the "rules" object. Labels like "correctness" or "stylistic" feel like just that: labels - they can be useful for the user when deciding which rules to enable or disable, but just as information. Anything else feels too complicated imo. |
Beta Was this translation helpful? Give feedback.
-
|
In the eslint world, there is a tendency to add new If you only allow |
Beta Was this translation helpful? Give feedback.
-
|
I understand the reasoning but still find it too complicated and, as a user, I don't think I'd appreciate the added complexity of having a matrix for the sake of a slightly more "correct" theoretical foundation or a slightly "semantically cleaner" organization. Pragmatically speaking, Really, I only care about being able to quickly use presets to focus on my project. Those who want more precise control over the rules will probably spend a bunch of time going almost rule by rule anyway, or similar to what you said they might just enable everything and then disable the rules that annoy them (this is what I currently do with Biome btw!). There's always the option to create multiple presets inside each group/category that match those "super categories" (correctness, etc), and I've proposed an approach that'd let users specify more than one preset if they want to. In practice though, I don't think this 2x2 matrix of rulesets will be appealing to most users, and its trade-offs will quickly become a pain for some. Just like the example I gave above:
|
Beta Was this translation helpful? Give feedback.
-
|
The more I think about it, the more I see a case for my proposal. I think it serves the purpose you're after as well, and having multiple presets that can be stacked for a category could be a nice way to achieve what you want without the added complexity of a matrix. E.g. It's explicit, you can still provide more general "recommended" and "all" options, and it's per-category meaning the pain I mentioned is no longer a problem. More fine-grained too e.g. "for base rules, I care about correctness and style, but for react I'll just go with the recommended preset, and then for jest I'll just enable all rules because I really wanna be strict about it". |
Beta Was this translation helpful? Give feedback.
-
|
Mental model is easier: if you understand object spread and JSON objects, you immediately understand what's gonna be the resolved config. More fine-grained: care however much or little you want about each category. More explicit: no need for cross-referencing domains AND groups to understand if a certain rule will be enabled. Still quite terse: while the "matrix" version necessarily means less content overall in the JSON file because of the implicit behavior, my version is still not too verbose and can be scanned quite easily while also being explicit. |
Beta Was this translation helpful? Give feedback.
-
|
Building on the v2 proposal and based on my reasoning here (please read first), I want to propose a simplified take. {
"linter": {
"enabled": true,
"presets": {
"base": "recommended",
"react": "recommended",
"jest": "stylistic",
"preact": ["correctness", "stylistic"]
},
"rules": {
"base/useConsistentArraySyntax": "warn",
"base/noSuperWithoutExtends": "off",
"react/useRulesOfHooks": "off",
"experimental/useSortedClasses": {
"level": "warn",
"prefix": "tw-"
}
},
"options": { ... }
}
}
Benefits of this approach:
Other ideas where the added complexity might not be worth it, but still worth considering IMO:
|
Beta Was this translation helpful? Give feedback.
-
|
Continuing the discussion here. I think you are right about the matrix thing: maybe it is too much of indirection. However, I could keep the central ideas of Proposal V2:
I like the idea of adding the domain to the name of the rule. I already thought about that. |
Beta Was this translation helpful? Give feedback.
-
I think we need to disambiguate a little bit. In my proposal, I use "X category" to mean "a set of rules related to X". For example, the "react" category is the set of rules related to React. In this sense, I think "domain" is a synonym, would you agree with this? Then, I understand "preset" as a pre-defined set of rule configurations. Using these definitions, presets are specific to certain categories. The "base" category contains general js-related rules, and has a "recommended" preset, as well as an "all" preset that configures all rules in the category. Then, there can be a preset for each "subcategory" or "type" or however you wanna call it: correctness, style, a11y, etc. Under these definitions, which I find to be the most intuitive, it makes sense to me that the config key is named "presets", because that's what you're enabling through the option. And while I think "category" is a more intuitive name than "domain", it doesn't seem to matter because I don't see a reason to use the concept in the config by name.
I think the problem with this is that we might not want to set all rules from all of those "rule types" as recommended. I can think of some that are too "pendantic" (borrowing from clippy terminology). I think it's simpler to think of all presets as flat entities, and of course any "non-type presets" like "recommended" should be understood to contain a combination of rules found across multiple "rule types", if this makes sense.
Yes, but I think my point above is an important bit of nuance. For example, I might want to enable the recommended rules AND all of the correctness ones, even the pedantic ones.
I agree with the approach and I think the nature of each "rule type" justifies the level in a self-evident way (like your example which is very reasonable). I don't think there's a need for an abstraction or special behavior here though: by having a flat concept of preset this convention can be applied manually in a very simple and explicit way. Just have a preset JSON with rule configs, and make sure that the levels are consistent with the established convention.
Cool! We're on the same page about this then :)
I don't think it's the most critical thing, but I also think it's a really good chance to improve it. I believe being explicit about nursery rules being "experimental" - which is widely understood by most developers as a concept with implications around "use at your own risk" and lack of stability - could solve some of the frustrations and pains we've observed and allow us to be more liberal with unstable code. One example of this is that we could enable the I agree that this is a good moment to make a change like this since it'd be another breaking change to add to the batch. One last thought I just had. Biome wants to lint more than just JavaScript right? I'm thinking that, instead of "base", we could just have a category/domain for general rules corresponding to each language or family of languages. For example, "javascript" (js, ts, jsx, tsx), "json" (json, jsonc, etc), "css" (css, sass, etc). |
Beta Was this translation helpful? Give feedback.
-
We internally use the name
As I said in the analysis: Almost all the rules of |
Beta Was this translation helpful? Give feedback.
-
|
I am hesitant about If we start using it, there's a lot of teaching materials and support we need to provide. What's idiomatic? What's idiomatic in this "context"? What's idiomatic in JavaScript/TypeScript? JavaScript is an old language, so there are tons of opinions and schools of thought. Not sure if there's space for an idiomatic style |
Beta Was this translation helpful? Give feedback.
-
|
Another possibility is to move some recommended style rules in |
Beta Was this translation helpful? Give feedback.
-
|
This one is very superficial, but how do we feel about renaming the top-level keys using a simpler and more consistent pattern? There's currently a bit of a mix between names that refer to a part of Biome (linter, formatter), names that describe what is being interacted with (vcs, files), and then there's
Another lengthier but still consistent alternative is using gerund forms, e.g. |
Beta Was this translation helpful? Give feedback.
-
|
As @ematipico suggested, we could use the |
Beta Was this translation helpful? Give feedback.
-
|
Just wanted to add one idea to this because I just started evaluating Biome and this came up - ability to configure ignoring files for specific linting rules - something like this (using the current config format, but I think you get what I'm going for): {
"linter": {
"rules": {
"recommended": true,
"suspicious": {
"noExplicitAny": {
"ignore": ["**/*.spec.ts"]
}
}
}
}
}This would be nice to have especially for our test files, which we do want linted, but just a relaxed version because we want some flexibility with our tests. We can do this now with the disable comments, but all of our test files have to have the comments, and it would be a nice improvement to have this configured in one place. |
Beta Was this translation helpful? Give feedback.
-
|
Can't you use |
Beta Was this translation helpful? Give feedback.
-
|
Ah - I didn't even realize that. I guess the "overrides" pattern didn't click for me. |
Beta Was this translation helpful? Give feedback.
-
|
Is there a formatting option similar to the If not, are there any plans to implement one? In case anyone is unfamiliar with the rule I'm talking about, what I'd like is the option to turn this: if (condition) {
doSomething();
} else {
doSomethingElse();
}into this: if (condition) {
doSomething();
}
else {
doSomethingElse();
} |
Beta Was this translation helpful? Give feedback.
-
Background
Our database of lint rules is growing, and we have rules that are meant to be used in specific cases, languages, libraries and more. Managing so many rules can become a burden for a user, so Biome should provide more tools for controlling those rules, and the documentation should be more clear.
On top of that, we have a very deep configuration:
{ "linter": { "rules": { "a11y": { "noBlankTarget": "off" } } } }Even though we have the JSON schema, it takes a while to turn off a rule.
Goals
categoryfrom the user-facing APIs, but keep them internally;Drawbacks
Remove
rulesand groups from the configuration{ "linter": { "ignore": ["./dist"], + "noBlankTarget": "off", - "rules": { - "a11y": { - "noBlankTarget": "off" - } - } } }Internally, we can still map
noBlankTargetto the group where it belongs because the name of the rules are unique. Still, thenurserygroup should be kept, to make it harder for user to opt-in. Reason? Users needs to be aware that they are using rules that are a work in progress.Additionally, we will change our diagnostic categories as a result. Although the nursery rules will keep the
nursery/group in the category name:linter.ruleSetsIntroduction of
ruleSets. A new option that will allow users to pull rules that belong to different "concepts". For example a user could pull (turn on), rules that belong to our recommended rules, accessibility rules, JSX rules, react hook rules, rules around testing and rules specific to TS projects.:{ "linter": { "ruleSets": { "recommended": true, "a11y": true, "jsx": true, "reactHooks": true, "test": false, "ts": true } } }This will replace
linter.rules.recommendedandlint.rules.allby providing the same functionality:{ "linter": { - "rules": { - "recommended": true - } + "ruleSets": { + "recommended": true + } }{ "linter": { - "rules": { - "all": true - }, + "ruleSets": { + "all": true + } } }Important
Having rules sets defined as an object will allow to opt in or opt out.
With this strategy, we also drop the incompatibility with this rule sets, meaning that
"all"will take over every existing rule sets. We leave the decision to the user and ignore that they decide to do.Rules can belong to multiple rule sets. For example, the rule
noBlankTargetcan be long to therecommendedrule set and thea11yrule set.Rule sets will be part of the metadata of a rule. This will allow us to know in advance this information. With this information, we can create a more grain fined documentation of our rules.
projectA new top level configuration to better configure the project. In this new configuration, biome can enable more optionated features. For example, we can set a kind of project like
"react"and Biome will enable some rule sets as a result:jsx,recommended,testandreactHooks:{ "project": { "kind": ["react"] } }One could also add
"tailwind", and Biome will add additional formatting rules for aroundclassNamein JSX:{ "project": { "kind": ["react", "tailwind"] } }Beta Was this translation helpful? Give feedback.
All reactions