Skip to content
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

make unused variables a warning in eslint #8832

Merged
merged 4 commits into from
Oct 19, 2023
Merged

make unused variables a warning in eslint #8832

merged 4 commits into from
Oct 19, 2023

Conversation

FredKSchott
Copy link
Member

Changes

  • as the title says, turn this error into a warning
  • reason: Unused imports. Unused imports are a common occurrence during development. But in this repo, unused imports are automatically removed via our auto-formatter post-merge, so this isn't actually an issue for us. Treating these as errors creates distraction in the flow of development where the developer has to stop and resolve the error to make the file "green"/"good".

An alternative is to move this responsibility to tsc via noUnusedLocals and noUnusedParameters, which also treats unused imports as warnings automatically (in VSCode, at least) but keeps the rest as errors. But conceptually I'd prefer to keep linting and building under separate tools (eslint and tsc) so I'd only recommend that change if others feel uneasy about unused variables not triggering CI.

Testing

  • N/A

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2023

⚠️ No Changeset found

Latest commit: 53b27b8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@bluwy
Copy link
Member

bluwy commented Oct 17, 2023

IIUC this also doesn't error on unused variables in general now? e.g. const something in the file that's never used, or function parameters that's not used. The formatter doesn't auto-fix it for us so I'm unsure about making this a warning. I've personally not hit by this much myself until I'm ready to push my changes which I'd have to clean up these dangling code anyways 🤔

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I'm in favor of this! It's mostly an annoyance and often times I end up keeping my unused variable and renaming it to _oldName anyway.

@bluwy
Copy link
Member

bluwy commented Oct 19, 2023

If there's two approvals to this, I won't block the PR! I think we can merge it then.

@natemoo-re natemoo-re merged commit 740c916 into main Oct 19, 2023
13 checks passed
@natemoo-re natemoo-re deleted the wip-eslint branch October 19, 2023 18:09
@FredKSchott
Copy link
Member Author

Cool, glad this seems useful! Like all things linting you can always revisit this if people end up hating it. My personal preference is always for less linting in OSS due to PR friction, but I'm just a visitor here these days!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants