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

Add autofixing of lint warnings for specific packages/files #5187

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Jan 22, 2025

Explanation

Currently, there are a ton of lint warnings, and it would be good to address them sooner rather than later.

The good thing is that many of these warnings can be autofixed — but only if they are turned into errors first. So, what we could do is open eslint.config.mjs, change all of the rules that are configured to produce warnings to produce errors instead ("warn" -> "error"), then run yarn lint:eslint --fix and take care of them in one fell swoop.

However, if we took this approach, approving such a PR would likely take a while since the changes would touch a bunch of packages in this monorepo and require codeowner approval from a bunch of teams.

Instead, it would be better if we batched lint violation fixes by codeowner. That is, open PRs progressively by addressing all lint violations for packages owned by the Wallet Framework team first, then the Accounts team, then the Confirmations team, etc.

To do this, we would need a way to run ESLint on specific directories. Again, that seems easy on its own, but we'd have to repeat the step that modifies eslint.config.mjs to convert warning-producing rules to errors instead each time we wanted to make a new PR.

Instead, if would be better if we could ask our ESLint script to not only allow custom file paths to be passed in, but also convert warnings into errors for us at the same time.

That's what this PR achieves. For instance, you should be able to say:

yarn lint:eslint packages/network-controller --treat-warnings-as-errors --fix

and now ESLint will run just on network-controller files, and autofix any warnings automatically.

Manual testing steps

As stated above, you should be able to run e.g.:

yarn lint:eslint packages/network-controller --treat-warnings-as-errors --fix

and see a bunch of changes made to network-controller files that fix lint violations. (ESLint may continue to report some errors as unfixable, but that's okay.)

References

Changelog

(N/A, developer-only change)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mcmire mcmire marked this pull request as ready for review January 30, 2025 15:56
@mcmire mcmire marked this pull request as draft January 30, 2025 15:57
@mcmire
Copy link
Contributor Author

mcmire commented Jan 30, 2025

Blocked by #5159. No longer blocked.

Currently, there are a ton of lint warnings, and it would be good to
address them sooner rather than later.

The good thing is that many of these warnings can be autofixed — if they
are turned into errors first. So, what we _could_ do is open
`eslint.config.mjs`, change all of the rules that are configured to
produce warnings to produce errors instead (`"warn"` -> `"error"`), then
run `yarn lint:eslint --fix` and take care of them in one fell swoop.

However, if we took this approach, approving such a PR would likely take
a while since the changes would touch a bunch of packages in this
monorepo and require codeowner approval from a bunch of teams.

Instead, it would be better if we batched lint violation fixes by
codeowner. That is, open PRs progressively by addressing all lint
violations for packages owned by the Wallet Framework team first, then
the Accounts team, then the Confirmations team, etc.

To do this, we would need a way to run ESLint on specific directories.
Again, that seems easy on its own, but we'd have to repeat the step that
modifies `eslint.config.mjs` to change warning-producing rules to
produce errors instead each time we wanted to make a new PR.

Instead, if would be better if we could ask our ESLint script to not
only allow custom file paths to be passed in, but also convert warnings
into errors for us.

That's what this PR does. For instance, you could say:

```
yarn lint:eslint packages/network-controller --treat-warnings-as-errors --fix
```

and now ESLint will run just on `network-controller` files, and autofix
any warnings automatically.
@mcmire mcmire force-pushed the per-package-lint-autofix branch from 4476f02 to 978ad9a Compare January 30, 2025 17:48
@mcmire mcmire changed the base branch from reorganize-warning-thresholds to main January 30, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant