-
Notifications
You must be signed in to change notification settings - Fork 176
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
Add quick fix for S4621 ('no-duplicate-in-composite') #3061
Conversation
e2b4f73
to
777bc11
Compare
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.
TBH I am not big fan of such solution based on full text replacement (and not on removal of duplicates), but indeed it's pretty straight-forward.
@@ -54,6 +55,7 @@ export const rule: Rule.RuleModule = { | |||
|
|||
groupedTypes.forEach(duplicates => { | |||
if (duplicates.length > 1) { | |||
const suggest = getSuggestions(compositeType, duplicates.slice(1), context); |
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.
minor but I would move slicing logic inside of the function
const firstToken = context.getSourceCode().getFirstToken(compositeNode); | ||
const lastToken = context.getSourceCode().getLastToken(compositeNode); | ||
let [prefix, suffix] = ['', '']; | ||
if (firstToken?.value === '(' && lastToken?.value === ')') { |
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 don't see test for this
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.
SonarJS/eslint-bridge/tests/rules/no-duplicate-in-composite.test.ts
Lines 164 to 198 in 777bc11
code: `type T = (number | string) & (number | string)`, | |
errors: [ | |
{ | |
suggestions: [ | |
{ | |
output: `type T = (number | string)`, | |
}, | |
], | |
}, | |
], | |
}, | |
{ | |
code: `type T = (number) | string | number`, | |
errors: [ | |
{ | |
suggestions: [ | |
{ | |
output: `type T = number | string`, | |
}, | |
], | |
}, | |
], | |
}, | |
{ | |
code: `type T = number | string | (number)`, | |
errors: [ | |
{ | |
suggestions: [ | |
{ | |
output: `type T = number | string`, | |
}, | |
], | |
}, | |
], | |
}, |
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.
hm... here first and last parentheses are not a pair, that's why I didn't get that it covers the condition
Then for (number | string) & (number | string) & Foo
we gonna get number | string & Foo
which is wrong.
const firstToken = context.getSourceCode().getFirstToken(compositeNode); | ||
const lastToken = context.getSourceCode().getLastToken(compositeNode); | ||
let [prefix, suffix] = ['', '']; | ||
if (firstToken?.value === '(' && lastToken?.value === ')') { |
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.
hm... here first and last parentheses are not a pair, that's why I didn't get that it covers the condition
Then for (number | string) & (number | string) & Foo
we gonna get number | string & Foo
which is wrong.
6a60372
to
e37edf0
Compare
SonarQube Quality Gate |
Fixes #3027