Add autofixing of lint warnings for specific packages/files #5187
Conversation
|
|
mcmire
force-pushed
the
per-package-lint-autofix
branch
from
4476f02 to
978ad9a
Compare
Currently, there are a ton of lint warnings, and it would be good to address them sooner rather than later. To make PRs easier to approve, we could batch 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 need a way to run ESLint on specific directories. That's what this PR does. Now you can say, for example: ``` yarn lint:eslint packages/network-controller --fix ``` and now ESLint will run just on `network-controller` files, and autofix any warnings automatically. One thing to keep in mind here is that we also want to keep the warning thresholds file up to date. This is a bit tricky because if we were to run ``` yarn lint:eslint packages/network-controller --quiet ``` then ESLint would only process lint errors and not warnings + errors. If this command is successful, i.e., there are no lint errors found, then we don't want the warning thresholds file to be blown away. So this commit also contains updates to the logic to ensure this doesn't happen.
mcmire
force-pushed
the
per-package-lint-autofix
branch
from
978ad9a to
245a4a0
Compare
| */ | ||
| async function runESLint( |
There was a problem hiding this comment.
The diff is a bit rough here. Essentially I folded the contents of the runESLint function back into main and then extracted printResults. I felt this slight refactor was necessary because it was easier to see where we are using the output from lintFiles (a list of files and lint rules that were checked, which have been filtered to the given set of files) and where we are using a version of that list that is further filtered to just include errors.
| console.log(chalk.underline(filePath)); | ||
| for (const [ruleId, count] of Object.entries(ruleCounts)) { | ||
| console.log(` ${chalk.cyan(ruleId)}: ${count}`); | ||
| } | ||
| } | ||
| saveWarningThresholds(warningCounts); |
There was a problem hiding this comment.
We now return a status from this function so we can simplify the nesting here.
| results, | ||
| }); | ||
|
|
||
| const completeWarningCounts = removeFilesWithoutWarnings({ |
There was a problem hiding this comment.
This is the key change to this function. If we are checking a selection of files and not the whole project then the set of lint warning counts we've tabulated will be a subset of what is already inside of the warning thresholds file, so we need to merge in what we have rather than completely overwrite the file. We also clean up the file by removing files that actually have no lint warnings whatsoever (see getWarningCounts below for why we now need to do this).
| const unsortedWarningCounts = results.reduce( | ||
| (workingWarningCounts, result) => { | ||
| const { filePath } = result; | ||
| const relativeFilePath = path.relative(PROJECT_DIRECTORY, filePath); | ||
| if (!workingWarningCounts[relativeFilePath]) { |
There was a problem hiding this comment.
This change is necessary now that we are always merging what this function returns with what already exists in the warning thresholds file (see above).
If all of the lint violations that are warnings are fixed within a file, we want to include that file path in the object that this function returns because otherwise the counts won't get reset for that file in the warning thresholds file and our script will still think that there are warnings.
| if (!quiet && !hasErrors) { | ||
| evaluateWarnings(results); | ||
| if (hasErrors || qualityGateStatus === QualityGateStatus.Increase) { | ||
| process.exitCode = 1; |
There was a problem hiding this comment.
I pulled this out of runESLint and what is now applyWarningThresholdsQualityGate because otherwise it was hard to see.
Explanation
Currently, there are a ton of lint warnings, and it would be good to address them sooner rather than later. To make PRs easier to approve, we could batch 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 need a way to run ESLint on specific directories. That's what this PR does. Now you can say, for example:
and now ESLint will run just on
network-controllerfiles, and autofix any warnings automatically.One thing to keep in mind here is that we also want to keep the warning thresholds file up to date. This is a bit tricky because if we were to run
then ESLint would only process lint errors and not warnings + errors. If this command is successful, i.e., there are no lint errors found, then we don't want the warning thresholds file to be blown away. So this commit also contains updates to the logic to ensure this doesn't happen.
Manual testing steps
As stated above, you should be able to run e.g.:
and see a bunch of changes made to
network-controllerfiles 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