-
Notifications
You must be signed in to change notification settings - Fork 562
build(build-tools): Enable unused variable linting #24272
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
Conversation
build-tools/packages/build-infrastructure/src/test/packageJsonUtils.test.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * A transformer function that does nothing. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars |
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.
Curious why do this here instead of the underscore prefix.
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 can't figure out why, but the lint rule gets tripped even when disabled in the config. I ended up just removing the placeholder args completely.
| import detectIndent from "detect-indent"; | ||
| // Imports are written this way for CJS/ESM compat | ||
| import fsePkg from "fs-extra"; | ||
| // eslint-disable-next-line import/no-named-as-default-member -- Imports are written this way for CJS/ESM compat |
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.
Are these just warnings and we're being good citizens and documenting them? Can't be that enabling the other rule somehow made this one start complaining, right?
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.
Yeah, they're just warnings, but I noticed that there was a comment already so I converted it to a disable with the explanation.
alexvy86
left a comment
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.
Looks good from my end
The unused variable eslint rule was not enabled in build-tools and the tsconfig noUnusedLocals option was disabled (by design), so unused variables were not caught before merge.
This change updates the lint config in build-tools to catch these issues in lint and makes the necessary changes to the source.