Add quick fix for S1172 ('no-unused-function-argument') #3038
Conversation
yassin-kammoun-sonarsource
requested review from
vilchik-elena
and removed request for
vilchik-elena
yassin-kammoun-sonarsource
force-pushed
the
issue-3033
branch
from
123db8f
to
7d7881f
Compare
| data: { | ||
| param: paramVariable.name, | ||
| }, | ||
| fix: fixer => fixer.replaceText(paramIdentifier, `_${paramVariable.name}`), |
There was a problem hiding this comment.
may be using insertTextBefore will be more efficient?
There was a problem hiding this comment.
I think it will not work for parameter with type. Can we have a test?
| fix: fixer => fixer.replaceText(paramIdentifier, `_${paramVariable.name}`), | ||
| }, | ||
| ]; | ||
| const funct = paramVariable.defs[0].node as TSESTree.FunctionLike; |
There was a problem hiding this comment.
why such name for variable?
There was a problem hiding this comment.
funct for function, but the latter is a keyword.
There was a problem hiding this comment.
I guess I will rename it to functionLike.
There was a problem hiding this comment.
ah, indeed:)) it's just usually we use fun or func. I have never seen funct
| @@ -196,5 +196,54 @@ ruleTester.run('Unused function parameters should be removed', rule, { | |||
| }`, | |||
| errors: 1, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
can we add unit test with parameter type to verify that type is also removed?
| fix: fixer => { | ||
| const paramIndex = funct.params.indexOf(paramIdentifier as TSESTree.Parameter); | ||
| if (funct.params.length === 1) { | ||
| return fixer.remove(funct.params[paramIndex] as estree.Node); |
There was a problem hiding this comment.
what will happen with trailing comma?
| } else if (funct.params.length - 1 === paramIndex) { | ||
| const [, start] = funct.params[paramIndex - 1].range; | ||
| const [, end] = funct.params[paramIndex].range; | ||
| return fixer.removeRange([start, end]); |
There was a problem hiding this comment.
this should be fine for trailing comma but can we add a test for it?
yassin-kammoun-sonarsource
changed the title
yassin-kammoun-sonarsource
force-pushed
the
issue-3033
branch
from
7d7881f
to
2a57001
Compare
| return fixer.replaceText(param, '()'); | ||
| } | ||
| } | ||
| case 'FunctionDeclaration': { |
There was a problem hiding this comment.
this case and next one are very similar, can't we generalize the logic?
| messages: { | ||
| removeOrRenameParameter: | ||
| 'Remove the unused function parameter "{{param}}" or rename it to "_{{param}}" to make intention explicit.', | ||
| suggestRemoveParameter: 'Remove {{param}} (beware of call sites)', |
There was a problem hiding this comment.
I think we should wrap parameter names into quotes.
| switch (func.type) { | ||
| case 'ArrowFunctionExpression': { | ||
| const param = func.params[paramIndex] as estree.Node; | ||
| const token = context.getSourceCode().getTokenAfter(param); |
There was a problem hiding this comment.
theoretically there could be comma ((a,) => foo())
| code: `function fun(a, b, c, d, {e}, f, g) { b = d = f; }`, | ||
| errors: [ | ||
| { | ||
| suggestions: [ |
There was a problem hiding this comment.
I think you don't have to assert all suggestions and their messages. Assert only what matters for the case, otherwise it's hard to follow
| const openingParenthesis = context | ||
| .getSourceCode() | ||
| .getTokenBefore(param, token => token.value === '('); | ||
| const closingParenthesis = context |
There was a problem hiding this comment.
this will not work for a => foo() :(
| suggestions: [ | ||
| { | ||
| desc: 'Rename "a" to "_a"', | ||
| output: '( _a, ) => {}', |
There was a problem hiding this comment.
I see we can't remove this suggestion from assertion, but let's at least remove the messages to reduce some readablility noise
Co-authored-by: Elena Vilchik <elena.vilchik@sonarsource.com>
|
SonarQube Quality Gate
|
Fixes #3033