fix: Refactor reporting into FileReport #19877

nzakas · 2025-06-19T19:10:41Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

Refactor

What changes did you make? (Give an overview)

I moved the logic for tracking lint errors/warnings into a new class called FileReport. This replaces the createReportTranslator() function, reducing the need to create a new report translator for each rule.

refs #18787

Is there anything you'd like reviewers to focus on?

I'm unsure of the method names of FileReport. I decided to go with:

addError/addErrors - add one or more errors
addWarning/addWarnings - add one or more warnings
addFatal/addFatals - add one or more fatal warnings
addRuleMessage - add a message from a rule

It seemed like having named methods was less error-prone than requiring severity to always be passed or making it optional and defaulting to error. I'm not sure if we should combine the methods so one method can add one or more messages (so just addError but use rest arguments?). Any thoughts on these methods would be appreciated.

netlify · 2025-06-19T19:10:46Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`15aef0a`
🔍 Latest deploy log	https://app.netlify.com/projects/docs-eslint/deploys/68839101bf8f6f0008a073b3
😎 Deploy Preview	https://deploy-preview-19877--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull Request Overview

This pull request refactors how lint errors and warnings are tracked by introducing a new FileReport class. It removes the creation of a per-rule report translator, consolidating message reporting, and updates tests accordingly.

Introduces FileReport to handle error, warning, and fatal messages.
Updates linter.js to use FileReport methods instead of pushing to arrays.
Adjusts tests to reflect changed message order and severity mappings.

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

File	Description
tests/lib/linter/linter.js	Updates test expectations and accidentally left a focused test.
lib/linter/linter.js	Integrates FileReport and refactors message reporting logic.

Comments suppressed due to low confidence (2)

tests/lib/linter/linter.js:8953

Remove 'describe.only' to ensure all tests run in the suite.

describe.only("Linter with FlatConfigArray", () => {

lib/linter/linter.js:411

Verify that mapping numeric severity 1 to a warning and 2 to an error is intentional and consistent with ESLint's severity semantics throughout the codebase.

		if (numericSeverity === 1) {

refs #18787

lib/linter/linter.js

tests/lib/linter/report-translator.js

lib/linter/linter.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

lib/linter/linter.js

lib/linter/file-report.js

lib/linter/linter.js

lib/linter/file-report.js

lib/linter/linter.js

tests/lib/linter/report-translator.js

tests/lib/linter/file-report.js

tests/lib/linter/report-translator.js

tests/lib/linter/file-report.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

tests/lib/linter/file-report.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

nzakas · 2025-07-02T18:45:41Z

I'd also like some feedback on the API itself. I'm not satisfied with it at present, but I'm also not sure how I'd change it.

mdjermanovic

LGTM if we want to keep this API.

mdjermanovic · 2025-07-03T13:02:57Z

I'd also like some feedback on the API itself. I'm not satisfied with it at present, but I'm also not sure how I'd change it.

Is there anything particular in the API you think could be improved?

It seemed like having named methods was less error-prone than requiring severity to always be passed or making it optional and defaulting to error. I'm not sure if we should combine the methods so one method can add one or more messages (so just addError but use rest arguments?). Any thoughts on these methods would be appreciated.

Named methods look good to me. There's currently only one place where the code needs to have a conditional to call either addError or addWarning due to not knowing the severity in advance. If more such cases occur, we could consider adding another, generic method addMessage(severity, descriptor).

nzakas · 2025-07-03T18:46:46Z

I feel like addError() and addErrors() is a bit clunky, maybe the methods should be merged into one?

Those methods and addRuleMessage() seem bit mismatched to me -- the different signatures, addRuleMessage() returns the message while the others don't.

I just feel like there could be a cleaner version of this API.

It also could just not matter at this point. 😄

mdjermanovic · 2025-07-04T13:39:46Z

I feel like addError() and addErrors() is a bit clunky, maybe the methods should be merged into one?

Maybe, but two methods, one accepting a descriptor object, the other accepting an array of descriptor objects, as they are implemented now, seem a good fit for the usage we have:

eslint/lib/linter/linter.js

Lines 469 to 472 in 8a46c15

    
           report.addError({ 
        
           	message, 
        
           	loc, 
        
           });

eslint/lib/linter/linter.js

Line 671 in 8a46c15

report.addErrors(directivesProblems);

Those methods and addRuleMessage() seem bit mismatched to me -- the different signatures, addRuleMessage() returns the message while the others don't.

I just feel like there could be a cleaner version of this API.

It also could just not matter at this point. 😄

I think it's fine as-is, at least for the start. I'd be in favor of merging this now since it's a fairly big diff so it could run into merge conflicts with other PRs, and we could revisit details at some later point.

nzakas · 2025-07-09T15:27:38Z

Decided to just clean things up a bit:

Eliminated addWarnings, addErrors, and addFatals. It's easy enough to use forEach when necessary to accomplish the same thing
Updated addWarning, addError, and addFatal to return the newly-created LintMessage to match addRuleMessage()

I think this makes the API feels more cohesive.

nzakas · 2025-07-11T15:38:12Z

@fasttime not critical to get this into the release but would appreciate a review.

mdjermanovic

LGTM, thanks!

fasttime

LGTM generally, just two questions.

fasttime · 2025-07-14T06:45:06Z

tests/lib/linter/file-report.js

- * @fileoverview Tests for createReportTranslator
- * @author Teddy Katz
+ * @fileoverview Tests for FileReport class
+ * @author ESLint


Not sure if this was intentional or AI-generated.

Haha yes, AI generated. I'll fix it.

fasttime · 2025-07-14T15:42:41Z

tests/lib/linter/linter.js

-										"test/my-rule",
+										null,
 									);
-									assert.strictEqual(messages[0].severity, 1);
+									assert.strictEqual(messages[0].severity, 2);
 									assert.strictEqual(
 										messages[0].message,
-										expectedRuleMessage,
+										"Unused inline config ('test/my-rule' is already configured to 'warn').",
 									);
+


I'm wondering if this change should be tagged as fix: instead of refactor: since it changes the order of messages in a lint report.

I'll mark the PR as a fix.

nzakas requested a review from a team as a code owner June 19, 2025 19:10

github-project-automation bot moved this to Needs Triage in Triage Jun 19, 2025

github-project-automation bot added this to Triage Jun 19, 2025

eslint-github-bot bot added the chore This change is not user-facing label Jun 19, 2025

github-actions bot added the core Relates to ESLint's core APIs and features label Jun 19, 2025

nzakas requested a review from Copilot June 19, 2025 19:10

Copilot AI reviewed Jun 19, 2025

View reviewed changes

refactor: Reporting into FileReport

3750885

refs #18787

nzakas force-pushed the file-report branch from 3d77f85 to 3750885 Compare June 19, 2025 19:13

Remove unused file

7e2164e

mdjermanovic reviewed Jun 20, 2025

View reviewed changes

lib/linter/linter.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jun 20, 2025

View reviewed changes

tests/lib/linter/report-translator.js Show resolved Hide resolved

mdjermanovic reviewed Jun 20, 2025

View reviewed changes

lib/linter/linter.js Show resolved Hide resolved

nzakas moved this from Needs Triage to Implementing in Triage Jun 20, 2025

nzakas and others added 3 commits June 20, 2025 16:44

Update tests

02d180e

Update lib/linter/linter.js

facf519

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Refactor runRules

b533ca3

mdjermanovic reviewed Jun 22, 2025

View reviewed changes

lib/linter/linter.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jun 22, 2025

View reviewed changes

lib/linter/file-report.js Outdated Show resolved Hide resolved

Don't pass language

04eb7fb

mdjermanovic reviewed Jun 24, 2025

View reviewed changes

mdjermanovic reviewed Jun 25, 2025

View reviewed changes

nzakas and others added 7 commits June 27, 2025 11:38

Update lib/linter/linter.js

e1b729e

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update lib/linter/linter.js

7c6619b

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update lib/linter/file-report.js

69a10fc

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update lib/linter/file-report.js

177614a

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update lib/linter/file-report.js

5f29b17

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update lib/linter/linter.js

fb2ab51

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

e79d7dd

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

nzakas and others added 8 commits July 1, 2025 16:28

Update tests/lib/linter/file-report.js

3e3076e

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

9ed4a76

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

a94ed0e

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

3da8ae7

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

e47cc0d

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

9b152b7

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Update tests/lib/linter/file-report.js

65c6408

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Clean up tests

baa0f40

mdjermanovic reviewed Jul 2, 2025

View reviewed changes

tests/lib/linter/file-report.js Outdated Show resolved Hide resolved

Update tests/lib/linter/file-report.js

8a46c15

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic previously approved these changes Jul 3, 2025

View reviewed changes

lumirlumir added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 7, 2025

Remove methods, return message from others

5ace576

nzakas dismissed mdjermanovic’s stale review via 5ace576 July 9, 2025 15:25

mdjermanovic previously approved these changes Jul 12, 2025

View reviewed changes

fasttime previously approved these changes Jul 14, 2025

View reviewed changes

Fix file comments

15aef0a

nzakas changed the title ~~refactor: Reporting into FileReport~~ fix: Refactor reporting into FileReport Jul 25, 2025

eslint-github-bot bot added the bug ESLint is working incorrectly label Jul 25, 2025

nzakas dismissed stale reviews from fasttime and mdjermanovic via 15aef0a July 25, 2025 14:13

nzakas merged commit bbf23fa into main Jul 25, 2025
30 checks passed

nzakas deleted the file-report branch July 25, 2025 14:21

github-project-automation bot moved this from Implementing to Complete in Triage Jul 25, 2025

Uh oh!

fix: Refactor reporting into FileReport #19877

fix: Refactor reporting into FileReport #19877

Conversation

nzakas commented Jun 19, 2025

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

Uh oh!

netlify bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nzakas commented Jul 2, 2025

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

mdjermanovic commented Jul 3, 2025

Uh oh!

nzakas commented Jul 3, 2025

Uh oh!

mdjermanovic commented Jul 4, 2025

Uh oh!

nzakas commented Jul 9, 2025

Uh oh!

nzakas commented Jul 11, 2025

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

fasttime left a comment

Choose a reason for hiding this comment

Uh oh!

fasttime Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

nzakas Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

fasttime Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

nzakas Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

netlify bot commented Jun 19, 2025 •

edited

Loading