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
fix(eslint-plugin): [consistent-type-imports] handle imports without specifiers #8308
fix(eslint-plugin): [consistent-type-imports] handle imports without specifiers #8308
Conversation
Thanks for the PR, @courageousillumination! 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 site configuration. |
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.
An excellent start for a first PR! Great 💖 emoji choice too, noticed & appreciated. 😄
Requesting changes on adding a unit test (and rule logic if needed) for the extra case mentioned.
packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks! Very clean & straightforward. Nice. 🙂
Adding 1 approval
and requesting review from @bradzacher because it coincidentally intersects with #8335.
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.
5a892c9
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8308 +/- ##
==========================================
+ Coverage 87.23% 87.24% +0.01%
==========================================
Files 251 251
Lines 12319 12319
Branches 3884 3884
==========================================
+ Hits 10746 10748 +2
+ Misses 1303 1302 -1
+ Partials 270 269 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
PR Checklist
Overview
Currently side effect imports will be marked as a
valueOnlyNamedImport
, which causes an error when trying insert the correct value import because it is expecting a}
.With these changes, we require that there is at least one specifier. If there is only a side effect import, a new import will be added with the specific value import.
One note is that this will also impact a named import with no specifiers
import {} from 'foo';
Previously this would have inserted the value import into the empty{}
; now it would insert it in a new import on a new line. I didn't see any way to distinguish between these two cases without looking for the braces explicitly; if that is not acceptable (since it changes how a fix is applied), please let me know and I can do the parsing.This is my first PR against this repository, so please let me know if I missed anything 💖