Skip to content

eslint: make --fix remove unused imports - #1076

Merged
phillco merged 6 commits into
mainfrom
unused-imports
Nov 18, 2022
Merged

eslint: make --fix remove unused imports#1076
phillco merged 6 commits into
mainfrom
unused-imports

Conversation

@phillco

@phillco phillco commented Oct 23, 2022

Copy link
Copy Markdown
Member

I noticed that eslint --fix didn't remove unused imports and eslint src --ext ts complained with a warning about an existing unused one:

$ eslint src --ext ts
.../src/core/commandVersionUpgrades/canonicalizeAndValidateCommand.ts
  1:10  warning  'window' is defined but never used. Allowed unused vars must match /^_/u  @typescript-eslint/no-unused-vars

I imagine this is because most people are fixing this up using the Visual Studio Code action, but it's nice to have everything working from the command line. Also JetBrains will only do it if eslint --fix does, hence my motivation :)

This adds the configuration such that eslint src --ext ts --fix removed unused imports, and fixes the existing unused imports in the codebase. Let me know if I made a mistake.

Checklist

@phillco
phillco requested a review from pokey as a code owner October 23, 2022 18:26
@phillco phillco changed the title eslint: make --fix remove unused imports eslint: make --fix remove unused imports Oct 23, 2022

@pokey pokey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I am definitely supportive of this direction

I think we need to tweak the ignore comments for most of these unused imports, though; see inline comment.

Also see #640; would be good to get this into pre-commit if easy

Comment thread src/typings/target.types.ts
Comment thread package.json
@phillco

phillco commented Oct 24, 2022

Copy link
Copy Markdown
Member Author

Yeah, I definitely did take a look at #640 :). A quick naive implementation had it making a bunch of changes, so I stopped for now.

Will update the tagging on the existing imports.

@phillco
phillco requested a review from pokey October 31, 2022 22:24
@phillco

phillco commented Oct 31, 2022

Copy link
Copy Markdown
Member Author

I've tagged the existing unused imports with unused-imports/no-unused-imports and restored them, and updated the branch. Will take a look at https://github.com/import-js/eslint-plugin-import next to see if it's better, but otherwise this version is ready to go.

@phillco
phillco requested a review from auscompgeek November 1, 2022 19:26
@phillco

phillco commented Nov 1, 2022

Copy link
Copy Markdown
Member Author

From the meetup: we looked at https://github.com/import-js/eslint-plugin-import briefly but didn't see a rule for removing unused imports. no-unused-modules seems to be more about finding exports in your own modules that aren't used. @auscompgeek let me know if I missed it.

If there is, we can use it. Otherwise, this should be ready to merge and we can take a look at adding some of eslint-plugin-import's rules as a follow up change, as I imagine that would be valuable in their own right.

@phillco

phillco commented Nov 6, 2022

Copy link
Copy Markdown
Member Author

@auscompgeek @pokey can we merge this?

@pokey pokey added the question Further information is requested label Nov 8, 2022

@pokey pokey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@phillco
phillco merged commit 371e74e into main Nov 18, 2022
@phillco
phillco deleted the unused-imports branch November 18, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants