Skip to content

Commit

Permalink
Add autofixing of lint warnings for specific packages/files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mcmire committed Jan 30, 2025
1 parent e5945d2 commit 978ad9a
Showing 1 changed file with 94 additions and 14 deletions.
108 changes: 94 additions & 14 deletions scripts/run-eslint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,34 @@ const EXISTING_WARNINGS_FILE = path.resolve(
'../eslint-warning-thresholds.json',
);

/**
* The parsed command-line arguments.
*/
type CommandLineArguments = {
/**
* Whether to cache results to speed up future runs (true) or not (false).
*/
cache: boolean;
/**
* A list of specific files to lint.
*/
files: string[];
/**
* Whether to automatically fix lint errors (true) or not (false).
*/
fix: boolean;
/**
* Whether to only report errors, disabling the warnings quality gate in the
* process (true) or not (false).
*/
quiet: boolean;
/**
* Whether to treat all warnings as errors so that they can be autofixed
* (true) or not (false).
*/
treatWarningsAsErrors: boolean;
};

/**
* An object mapping rule IDs to their warning counts.
*/
Expand All @@ -28,9 +56,14 @@ type WarningComparison = {
};

/**
* The warning severity of level of an ESLint rule.
* The severity level for an ESLint message.
*/
const WARNING = 1;
enum ESLintMessageSeverity {
Warning = 1,
// This isn't a variable.
// eslint-disable-next-line @typescript-eslint/no-shadow
Error = 2,
}

// Run the script.
main().catch((error) => {
Expand All @@ -42,14 +75,20 @@ main().catch((error) => {
* The entrypoint to this script.
*/
async function main() {
const { cache, fix, quiet } = parseCommandLineArguments();
const args = parseCommandLineArguments();
const { cache, fix, quiet, treatWarningsAsErrors } = args;

const eslint = new ESLint({ cache, fix });
const results = await runESLint(eslint, { fix, quiet });
const files = args.files.length > 0 ? args.files : ['.'];
const results = await runESLint(eslint, files, {
fix,
quiet,
treatWarningsAsErrors,
});
const hasErrors = results.some((result) => result.errorCount > 0);

if (!quiet && !hasErrors) {
evaluateWarnings(results);
if (args.files.length === 0 && !args.quiet && !hasErrors) {
checkAgainstAndUpdateWarningThresholds(results);
}
}

Expand All @@ -58,8 +97,10 @@ async function main() {
*
* @returns The parsed arguments.
*/
function parseCommandLineArguments() {
return yargs(process.argv.slice(2))
function parseCommandLineArguments(): CommandLineArguments {
const { cache, fix, quiet, treatWarningsAsErrors, ...rest } = yargs(
process.argv.slice(2),
)
.option('cache', {
type: 'boolean',
description: 'Cache results to speed up future runs',
Expand All @@ -76,24 +117,40 @@ function parseCommandLineArguments() {
'Only report errors, disabling the warnings quality gate in the process',
default: false,
})
.help().argv;
.option('treatWarningsAsErrors', {
type: 'boolean',
description: 'Treat all warnings as errors so that they can be autofixed',
default: false,
})
.help()
.string('_').argv;

// Type assertion: The types for `yargs`'s `string` method are wrong.
const files = rest._ as string[];

return { cache, fix, quiet, treatWarningsAsErrors, files };
}

/**
* Runs ESLint on the project files.
* Runs ESLint on the given files.
*
* @param eslint - The ESLint instance.
* @param files - The list of files to lint.
* @param options - The options for running ESLint.
* @param options.quiet - Whether to only report errors (true) or not (false).
* @param options.fix - Whether to automatically fix problems (true) or not
* (false).
* @param options.treatWarningsAsErrors - Whether to treat all warnings as
* errors (true) or not (false). This is most useful when combined with `files`
* so that all warnings for a single package can be autofixed in a single go.
* @returns A promise that resolves to the lint results.
*/
async function runESLint(
eslint: ESLint,
options: { quiet: boolean; fix: boolean },
files: string[],
options: { quiet: boolean; fix: boolean; treatWarningsAsErrors: boolean },
): Promise<ESLint.LintResult[]> {
let results = await eslint.lintFiles(['.']);
let results = await eslint.lintFiles(files);
const errorResults = ESLint.getErrorResults(results);

if (errorResults.length > 0) {
Expand All @@ -102,6 +159,26 @@ async function runESLint(

if (options.quiet) {
results = errorResults;
} else if (options.treatWarningsAsErrors) {
results = results.map((result) => {
return {
...result,
messages: result.messages.map((message) => {
return {
...message,
severity:
message.severity === ESLintMessageSeverity.Warning
? ESLintMessageSeverity.Error
: message.severity,
};
}),
errorCount: result.errorCount + result.warningCount,
warningCount: 0,
fixableErrorCount:
result.fixableErrorCount + result.fixableWarningCount,
fixableWarningCount: 0,
};
});
}

const formatter = await eslint.loadFormatter('stylish');
Expand Down Expand Up @@ -129,7 +206,7 @@ async function runESLint(
*
* @param results - The results of running ESLint.
*/
function evaluateWarnings(results: ESLint.LintResult[]) {
function checkAgainstAndUpdateWarningThresholds(results: ESLint.LintResult[]) {
const warningThresholds = loadWarningThresholds();
const warningCounts = getWarningCounts(results);

Expand Down Expand Up @@ -216,7 +293,10 @@ function saveWarningThresholds(warningCounts: WarningCounts): void {
function getWarningCounts(results: ESLint.LintResult[]): WarningCounts {
const warningCounts = results.reduce((acc, result) => {
for (const message of result.messages) {
if (message.severity === WARNING && message.ruleId) {
if (
message.severity === ESLintMessageSeverity.Warning &&
message.ruleId
) {
acc[message.ruleId] = (acc[message.ruleId] ?? 0) + 1;
}
}
Expand Down

0 comments on commit 978ad9a

Please sign in to comment.