feat(biome_js_analyze): implement useConsistentObjectDefinition rule
#5042
Conversation
github-actions
bot
added
A-CLI
CodSpeed Performance ReportMerging #5042 will not alter performanceComparing Summary
|
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_literals.rs
Outdated
Show resolved
Hide resolved
| RuleDiagnostic::new(rule_category!(), node.range(), markup! {{title}}).note( | ||
| markup! { "Using a consistent object literal syntax makes it easier to anticipate the structure of an object." }, | ||
| ), |
There was a problem hiding this comment.
Our diagnostics must match our rule pillars. In this case, we don't have a message for the error, which should be the first one.
There was a problem hiding this comment.
Yes but that part is the note, I used the title for the error, or does this not count as saying what the error is?
let title = match options.syntax {
ObjectLiteralSyntax::Shorthand => {
"Use shorthand object property syntax whenever possible."
}
ObjectLiteralSyntax::Explicit => {
"Always use explicit object property assignment syntax."
}
};There was a problem hiding this comment.
When I read "Use shorthand object property, it tells the user how to solve the error, which is the third pillar.
There was a problem hiding this comment.
Agreed, just updated it. I believe the title now covers the first pillar and the note does a good job at covering both the second and third, while a code action is not implemented.
That should be everything, let me know what you think!
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_literals.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_literals.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_literals.rs
Outdated
Show resolved
Hide resolved
dy0gu
changed the title
useConsistentObjectLiteralsuseConsistentObjectDefinition
dy0gu
changed the title
useConsistentObjectDefinitionuseConsistentObjectDefinition rule
There was a problem hiding this comment.
A couple of questions and considerations:
- The rule doesn't have any severity, is it intended? FYI rules without severity will have a
Informationseverity - There many cases that aren't covered by the tests. Do you intend to implement them later or now in this PR?
| /// ### syntax | ||
| /// | ||
| /// The syntax to use: | ||
| /// - `explicit`: enforces the use of explicit object property syntax in every case | ||
| /// - `shorthand`: enforces the use of shorthand object property syntax when possible | ||
| /// | ||
| /// **Default:** `explicit` |
There was a problem hiding this comment.
We would appreciate if you could provide an example with syntax: "shorthand". Our infrastructure allows you to do that, here the contribution guide that explains how to set it up: https://github.com/biomejs/biome/blob/main/crates/biome_analyze/CONTRIBUTING.md#using-rule-options
| #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields, default)] | ||
| pub struct UseConsistentObjectDefinitionOptions { | ||
| syntax: ObjectPropertySyntax, |
There was a problem hiding this comment.
Options must be documented. These are exposed to our users via JSON Schema, so we need to provide a good description.
| #[default] | ||
| Explicit, | ||
| Shorthand, |
There was a problem hiding this comment.
Same here, let's document the variants.
| const obj = { | ||
| foo: foo, | ||
| bar: function () { return "bar"; }, | ||
| }; |
There was a problem hiding this comment.
The ESLint rule has an extensive set of tests, we would appreciate we could port some of them.
For example, these cases aren't covered:
const a = {
"foo": foo, // literal
"foo": "foo", // right literal,
['foo']: foo, // computed properties
}
Summary
Related: #4816
This PR adds a rule inspired by object-shorthand from eslint. It is not a complete port.
I would also like to add a code action in the future, shouldn't be too hard. I may open a new PR for that but at the moment it would be too time consuming to look into.
Test Plan
Snapshot tests for both the options supported by the rule can be found in
crates\biome_js_analyze\tests\specs\nursery\useConsistentObjectLiterals.