-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat(biome_js_analyze): implement useConsistentObjectDefinition
rule
#5042
feat(biome_js_analyze): implement useConsistentObjectDefinition
rule
#5042
Conversation
CodSpeed Performance ReportMerging #5042 will not alter performanceComparing Summary
|
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.
Overall the code looks good, I left some suggestion. I think we settled on a different name
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
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
useConsistentObjectLiterals
useConsistentObjectDefinition
useConsistentObjectDefinition
useConsistentObjectDefinition
rule
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.
A couple of questions and considerations:
- The rule doesn't have any severity, is it intended? FYI rules without severity will have a
Information
severity - There many cases that aren't covered by the tests. Do you intend to implement them later or now in this PR?
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_definition.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_definition.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_definition.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/useConsistentObjectDefinition/invalidExplicit.js
Outdated
Show resolved
Hide resolved
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.
I think all comments were addressed here, right?
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_definition.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_object_definition.rs
Outdated
Show resolved
Hide resolved
I will merge the PR after I fix the conflicts and rework the changeset. As it is now, it's poor because it doesn't say anything to our users. |
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.
Thank you very much, @dy0gu, for your contribution. I hope you had fun :)
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
.