-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): add a default-off option to autofix remove unused imports #11243
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, @nayounsang! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
View your CI Pipeline Execution ↗ for commit 90c2d9a.
☁️ Nx Cloud last updated this comment at |
Wow, when adding a new feature, a follow-up development is needed. I'll work on it tomorrow. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11243 +/- ##
==========================================
+ Coverage 90.85% 90.86% +0.01%
==========================================
Files 501 501
Lines 50901 51013 +112
Branches 8383 8418 +35
==========================================
+ Hits 46248 46355 +107
- Misses 4638 4642 +4
- Partials 15 16 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts
Outdated
Show resolved
Hide resolved
This reverts commit e5394b2.
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 great start!
I'm leaving a bit of a drive-by review since I see that there are several major TODOs in the code that need to get resolved. Feel free to reach out if you're looking for help with those! In the meantime, I'm going to leave a Changes Requested review so that this isn't in our ready-to-review queue.
Thanks!
|
||
const source = context.sourceCode; | ||
const node = def.node; | ||
const decl = node.parent as TSESTree.ImportDeclaration; |
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 that this will be unsound for import = require
-style imports, for example
import x = require('foo')
Let's be sure to include some test coverage with that style of import
.
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.
Oh, nice catch. This is TSImportEqualsDeclaration
type.
I test with this code:
{
code: `
import x = require('foo');
import y = require('bar');
export { y };
`,
errors: [{ messageId: 'unusedVar' }],
options: [{ enableAutofixRemoval: { imports: true } }],
output: `
import y = require('bar');
export { y };
`,
}
In the existing logic, an error occurs and a more accurate logic is used rather than type casting.
related commit: 11ac4fa
@@ -687,6 +708,80 @@ export default createRule<Options, MessageIds>({ | |||
data: unusedVar.references.some(ref => ref.isWrite()) | |||
? getAssignedMessageData(unusedVar) | |||
: getDefinedMessageData(unusedVar), | |||
fix: options.enableAutofixRemoval?.imports |
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.
This seems useful as a suggestion, even if the autofix option is disabled. Consider using getFixOrSuggest
to conditionally provide this fix as a suggestion?
export function getFixOrSuggest<MessageId extends string>({ | |
fixOrSuggest, | |
suggestion, | |
}: { | |
fixOrSuggest: 'fix' | 'none' | 'suggest'; | |
suggestion: TSESLint.SuggestionReportDescriptor<MessageId>; | |
}): | |
| { fix: TSESLint.ReportFixFunction } | |
| { suggest: TSESLint.SuggestionReportDescriptor<MessageId>[] } | |
| undefined { | |
switch (fixOrSuggest) { | |
case 'fix': | |
return { fix: suggestion.fix }; | |
case 'none': | |
return undefined; | |
case 'suggest': | |
return { suggest: [suggestion] }; | |
} | |
} |
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.
Good idea. However, it would be better to provide suggestions to existing test cases and then resolve this comment before processing.
if (decl.specifiers.length === 1) { | ||
return fixer.removeRange([ | ||
decl.range[0], | ||
decl.range[1] + 1, // +1 to include "\n" |
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.
to include "\n"
Why do we want to do this exactly? This causes a bug with this technically valid code (please include as a test case - hint, you'll need to use noFormat
):
import x from 'foo';import y from 'bar'
My instinct is to say we're better off just leaving a blank line and letting the user decide how to format it afterwards (whether by hand or with prettier or similar).
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're better off just leaving a blank line and letting the user decide how to format it afterwards (whether by hand or with prettier or similar).
Wow, that's right. That should have been it. Plus, it lowers the difficulty of solving the problem and the code becomes simpler.
related commit: 9f76441 & 633d2c8
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.
packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts
Outdated
Show resolved
Hide resolved
@kirkwaiblinger Hi. I committed my code about your review and TODOs! Please make sure I'm on the right track. // test case
import {A,B,C,D,E,F,G,Used1,H,I,J,K} from 'foo';
export {Used1};
// expect
import {Used1} from 'foo';
export {Used1};
// received
import {B,D,F,Used1,I,K} from 'foo'
export {Used1}; How can I solve this problem? My idea is to merge the contiguous segments and remove multiple specifiers at once. import {A,B,C,D,E,F,G,Used1,H,I,J,K} from 'foo'; // ->
import {Used1,H,I,J,K} from 'foo'; // ->
import {Used1} from 'foo'; |
PR Checklist
Overview
Auto fix unused import statements
examples
Currently logically impossible (commented test cases)
examples
Guessing about the failed(commented) test case: remove multiple unused vars in one-line