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
[consistent-type-imports] restrict report to the specific offending specifier #4915
Comments
The fifteen levels of indentation in that rule's source makes me want to instantly quit coding every time I look at it |
Yeah the fixer is dog ugly logic to implement unfortunately because imports are so damn complex. Most of the rule logic isn't THAT bad though. It probably needs a pass over it to rewrite it now that it's written and finalised. |
I think that this kind of implies having a separate loc for each reported import, too, for example
This is kinda of problematic, until #8554 is closed (which looks to be very soon!), due to the amount of fixer logic in the rule. With multipass fixes, I think we can just have each symbol report and fix "individually" but still assert that the actual result is correct |
TBH idk if we should do individual reporting if there are multiple per import. |
I think we definitely should! |
@Josh-Cena the issue with multiple report locations is that it creates multiple fixers and those fixers cannot talk to one another - so either they will conflict or they'll do the same thing For example imagine this import { a, b, c } from "outside-module";
^ ^ If we want to fix this to an inline style it's easy - for sure: // fix 1
-import { a, b, c } from "outside-module";
+import { type a, b, c } from "outside-module";
// fix 2
-import { a, b, c } from "outside-module";
+import { a, b, type c } from "outside-module"; These fixes are non-overlapping so easy as. // fix 1
-import { a, b, c } from "outside-module";
+import { b, c } from "outside-module";
+import type { a } from "outside-module";
// fix 2
-import { a, b, c } from "outside-module";
+import { a, b } from "outside-module";
+import type { c } from "outside-module"; Now you have this issue either
Either way - that's not a great result! A slower lint run or duplicate code. IMO that's not a great trade-off for the end user for what is a relatively minor improvement. |
Has the idea of a non-contiguous loc for a single report been thought about before? That would allow us to get semantically sensible underlines but one fix... Would also make sense for |
I don't think it has - it's a pretty fundamental change to how eslint does things. I think there's huge value in it eg rust's clippy does multi range reports (but only for "related spans", not primary ones) and it's a great way to expose additional info to users. |
What about this: we still uphold the principle of "report what will be changed".
|
Repro
playground
Expected Result
The rule only reports the first
ImportSpecifier
Actual Result
The rule reports the entire
ImportDeclaration
, which makes debugging a little harder when the declaration is long.I'm not using autofix, because #4338 is not there yet.
Additional Info
Versions
@typescript-eslint/eslint-plugin
5.22.0
@typescript-eslint/parser
5.22.0
TypeScript
4,6.2
ESLint
8.14.0
node
18.0.0
The text was updated successfully, but these errors were encountered: