Change Request: Slight adjustment to logic introduced in #16348

[scalvert]

ESLint version

^8.25.0

What problem do you want to solve?

⚠️ I want to start by quickly saying that I acknowledge this issue I'm identifying is only manifesting due to our unorthodox usage of formatters. That said, I believe we can arrive at a reasonable solution that will maintain the current logic, while restoring the prior behavior in our formatter.

The PR #16348 added the ability to pass --max-warnings to formatters, which is a great addition. There's nothing inherently problematic about the solution apart from the execution ordering.

Our formatter, eslint-formatter-todo, provides a mechanism whereby you can defer fixing errors until a later date. This allows for organizations that have huge codebases to roll out new lint rules without breaking tons of consumers, and has become an invaluable part of our code maintenance workflows.

As part of this formatter, we modify the results to contain a new severity: todo. This allows us to indicate to the formatter whether to output errors or not - if they're todo, we ignore them unless an env var is set.

After the following change:

eslint/lib/cli.js

Lines 432 to 464 in 69216ee

	const resultCounts = countErrors(results);
	const tooManyWarnings = options.maxWarnings >= 0 && resultCounts.warningCount > options.maxWarnings;
	const resultsMeta = tooManyWarnings
	? {
	maxWarningsExceeded: {
	maxWarnings: options.maxWarnings,
	foundWarnings: resultCounts.warningCount
	}
	}
	: {};
	if (await printResults(engine, resultsToPrint, options.format, options.outputFile, resultsMeta)) {
	// Errors and warnings from the original unfiltered results should determine the exit code
	const shouldExitForFatalErrors =
	options.exitOnFatalError && resultCounts.fatalErrorCount > 0;
	if (!resultCounts.errorCount && tooManyWarnings) {
	log.error(
	"ESLint found too many warnings (maximum: %s).",
	options.maxWarnings
	);
	}
	if (shouldExitForFatalErrors) {
	return 2;
	}
	return (resultCounts.errorCount || tooManyWarnings) ? 1 : 0;
	}
	return 2;
	}

the order of when errors were counted has changed. This has resulted in the exit code of the CLI returning non-zero, since the count of the results occurs before the formatter is run rather than what it previous was, which was after.

What do you think is the correct solution?

If we could work the logic back to where the results count occurs after the formatters run, that would solve our problem. I'm totally happy to contribute this fix.

Participation

• I am willing to submit a pull request for this change.

Additional comments

Just want to also tag @btmills to get their thoughts, since they were the one who implemented this change!

[mdjermanovic]

Hi @scalvert, thanks for the issue!

If we could work the logic back to where the results count occurs after the formatters run, that would solve our problem.

Formatters aren't really expected to modify the results, so there are many ways to break this. For example, we might decide to clone or freeze the results at some point to prevent accidental changes. I think a better course of action would be to explore other ways to solve the problem that eslint-formatter-todo aims to solve.

As for a temporary workaround, since the formatter modifies the results to alter eslint's exit code calculations, maybe you could do process.exit(code) directly in the formatter instead. That's not really what formatters should be doing, but they shouldn't modify the results to affect the exit code either.

I’m curious what other team members think about this.

[scalvert]

Hey @mdjermanovic, thanks for taking the time to reply.

Formatters aren't really expected to modify the results, so there are many ways to break this. For example, we might decide to clone or freeze the results at some point to prevent accidental changes.

This is why my issues is prefaced with a big fat ⚠️ to indicate my awareness that we're deviating from what would be considered normal.

As for a temporary workaround, since the formatter modifies the results to alter eslint's exit code calculations, maybe you could do process.exit(code) directly in the formatter instead.

This is absolutely an option for us, but I was trying to minimize the formatter's fully escaping from the flow of the CLI. We can for sure use this as a workaround and put a comment in the code as to the why of it.

Either way, it is possible for us to update the logic so that the call ordering would be similar to what it was before, though as you've suggested that's not going to guarantee future operability.

[tylerbecks]

+1 I'm a consumer of eslint-formatter-todo and would love to see this ticket come to life. The amount of warnings we have is unwieldy and having --quiet would help us so much.

[mdjermanovic]

In the 2022-12-01 TSC meeting, we discussed this issue and decided not to make any changes. It is necessary to count warnings before running the formatter in order to pass additional data (context.maxWarningsExceeded) to the formatter. We'll not recalculate the data after running the formatter because formatters are not expected to modify lint results and affect the exit code.