feat(rome_js_analyze): noDuplicateCase, no-duplicate-case #3969 #4039
Conversation
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
| for case in node.cases() { | ||
| if let AnyJsSwitchClause::JsCaseClause(case) = case { | ||
| if let Ok(test) = case.test() { | ||
| let text = test.text(); |
There was a problem hiding this comment.
is it enough to compare text? Or we need to travers expression tree?
Can we get text without inner comment?
For example:
switch (a) {
case f(
a + 1 // comment
).p1:
break;
case f(a + 1).p1:
break;
default:
break;
}
There was a problem hiding this comment.
This is similar to the check being performed in #4031, maybe there's an opportunity to move this comparison in a shared utility for these two rules
There was a problem hiding this comment.
Do we want to do that?
It seems to me this would be case for another lint rule: to only allow constants as the case.
This will fall into the same cases as the binary: these expression can have side effects (or throw) and we will be changing the application behaviour.
There was a problem hiding this comment.
I took test cases from the eslint rule.
https://github.com/eslint/eslint/blob/main/tests/lib/rules/no-duplicate-case.js
It has this test case.
0bff894 to
74aca10
Compare
|
@denbezrukov could you please comment on #3969 so I can assign the issue to you? |
crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_case.rs
Outdated
Show resolved
Hide resolved
# Conflicts: # crates/rome_js_analyze/src/analyzers/nursery.rs # crates/rome_service/src/configuration/linter/rules.rs
95b88f1 to
419d1b8
Compare
crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_case.rs
Outdated
Show resolved
Hide resolved
3563f0c to
55f7b79
Compare
# Conflicts: # crates/rome_js_analyze/src/analyzers/nursery.rs # crates/rome_service/src/configuration/linter/rules.rs
Summary
Fix #3969
Test Plan
cargo test