-
-
Notifications
You must be signed in to change notification settings - Fork 415
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_analyzer): useConsistentStringFunctions
#3038
feat(biome_js_analyzer): useConsistentStringFunctions
#3038
Conversation
CodSpeed Performance ReportMerging #3038 will degrade performances by 6.36%Comparing Summary
Benchmarks breakdown
|
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.
Sorry if review is bit early, few suggestions
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/useConsistentStringFunctions/invalid.js
Outdated
Show resolved
Hide resolved
@minht11 |
@minht11 |
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
} | ||
|
||
fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> { | ||
let node = ctx.query(); |
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.
We are duplicating work a bit, lets find name node in diagnostic and pass it in state so you can directly replace it here. You can also check if fix should be available in diagnostic too, for example if substring has arguments and so on.
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.
@minht11
Thank yo for your suggestion. Yes, I will refactor this later:)
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/useConsistentStringFunctions/valid.js
Show resolved
Hide resolved
crates/biome_js_analyze/src/lint/nursery/use_consistent_string_functions.rs
Outdated
Show resolved
Hide resolved
We agreed to use the |
Ok, I will use methods, instead of functions. |
Close this PR due to a rule separation: |
Summary
Fixes #2817
Test Plan