Skip to content

fix: report messages for configuration comments correctly#618

Open
andreahlert wants to merge 3 commits intoeslint:mainfrom
andreahlert:fix/report-unused-disable-directives
Open

fix: report messages for configuration comments correctly#618
andreahlert wants to merge 3 commits intoeslint:mainfrom
andreahlert:fix/report-unused-disable-directives

Conversation

@andreahlert
Copy link

Summary

Previously, ESLint messages that pointed to configuration comments (such as unused disable directive warnings) were being silently dropped because the adjustMessage function returned null for any message pointing to lines before the actual code content.

This change:

  • Tracks the original HTML comment positions when collecting configuration comments from the Markdown
  • Builds a mapping from linted code line numbers to original comment info
  • Maps messages pointing to configuration comment lines back to their original HTML comment locations in the Markdown file

This fixes the issue where --report-unused-disable-directives would not work with the markdown processor, as those warnings were being filtered out.

Before

$ eslint --report-unused-disable-directives README.md
# (no output even when there are unused disable directives)

After

$ eslint --report-unused-disable-directives README.md
README.md
  3:1  warning  Unused eslint-disable directive (no problems were reported from 'no-undef')

Test plan

  • Added new test suite reportUnusedDisableDirectives with tests for single-line and multi-line HTML comments
  • All existing 1590 tests pass
  • Lint passes

Fixes #115

@eslint-github-bot eslint-github-bot bot added the bug label Feb 5, 2026
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 5, 2026

CLA Signed

The committers listed above are authorized under a signed CLA.

@andreahlert
Copy link
Author

Hi @nzakas, this fixes a 7-year-old issue (#115) where --report-unused-disable-directives silently drops warnings from HTML comments in Markdown files. CI is green and I've added 4 test cases covering the mapping logic. Would appreciate a review when you get a chance!

@andreahlert
Copy link
Author

Thanks for the review, @Ari4ka!

@nzakas
Copy link
Member

nzakas commented Feb 9, 2026

@andreahlert thanks, I'll take a look when I'm able.

Did you use AI to generate this PR?

@andreahlert
Copy link
Author

Hey @nzakas . Everybody says that to me 😅 I write lots of docs at work so it's a habit to be super detailed. And I usually generate an AI template of PR (for headers for exemple), to keep the same looking, but its all my writing.

I'll keep PR descriptions shorter going forward. Lmk if you want me to clarify anything about the implementation!

@DMartens
Copy link
Contributor

Thank you for submitting a PR for this. It is a really good starting for fixing this.

@DMartens DMartens moved this from Needs Triage to Implementing in Triage Feb 11, 2026
@lumirlumir
Copy link
Member

Hi — there's a CI failure. If you have a moment, could you take a look?

andreahlert added a commit to andreahlert/markdown that referenced this pull request Feb 23, 2026
Lint was failing because report-unused-disable-directives now correctly
reports warnings in .md files; tests/fixtures/long.md has intentional
directives that trigger those warnings. Align with main config which
already ignores tests/fixtures.

Refs eslint#618
@andreahlert
Copy link
Author

Hi — there's a CI failure. If you have a moment, could you take a look?

Done! Ready for final review.

@andreahlert
Copy link
Author

@nzakas @DMartens can we have a look on this?


export interface BlockBase {
baseIndentText: string;
comments: string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments property should be removed as it is no longer set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double-check this? It seems like comments is still used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the current implementation still uses the comments property. Otherwise it throws a type error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, comments is still used in getBlockRangeMap, block.comments.join for assembling linted text, and leadingCommentLines calculation. Kept it as-is.

@DMartens
Copy link
Contributor

DMartens commented Mar 5, 2026

Sorry for the delay. Other than the small things I mentioned, the changes LGTM.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me. I'd like to rename CommentInfo to something that better describes what it is. CommentWithLoc? MappedCommentLocation? Just something that makes it clear what it's doing.


export interface BlockBase {
baseIndentText: string;
comments: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double-check this? It seems like comments is still used.

Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase or merge from the main branch once? While reviewing the codebase, I noticed this PR is a bit behind HEAD, so I'm concerned some things may be out of sync when it's merged into main.

Also, it seems there are CI failures that need to be addressed before merging.


export interface BlockBase {
baseIndentText: string;
comments: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the current implementation still uses the comments property. Otherwise it throws a type error.

Previously, ESLint messages that pointed to configuration comments (such as
unused disable directive warnings) were being silently dropped because the
`adjustMessage` function returned `null` for any message pointing to lines
before the actual code content.

This change:
- Tracks the original HTML comment positions when collecting configuration
  comments from the Markdown
- Builds a mapping from linted code line numbers to original comment info
- Maps messages pointing to configuration comment lines back to their original
  HTML comment locations in the Markdown file

This fixes the issue where `--report-unused-disable-directives` would not work
with the markdown processor, as those warnings were being filtered out.

Fixes eslint#115

Signed-off-by: André Ahlert <andre@aex.partners>
Lint was failing because report-unused-disable-directives now correctly
reports warnings in .md files; tests/fixtures/long.md has intentional
directives that trigger those warnings. Align with main config which
already ignores tests/fixtures.

Refs eslint#618

Signed-off-by: André Ahlert <andre@aex.partners>
- Rename CommentInfo to MappedCommentLocation for clarity
- Import MappedCommentLocation type from ./types.js instead of inline @typedef
- Remove unnecessary * from wrapComment regex character sets
- Refactor tests to use beforeEach with shared config
- Add reportUnusedDisableDirectives tests under LegacyESLint describe block

Signed-off-by: André Ahlert <andre@aex.partners>
@andreahlert andreahlert force-pushed the fix/report-unused-disable-directives branch from e4ed80c to aaa9b03 Compare March 12, 2026 07:48
@andreahlert
Copy link
Author

@nzakas Renamed CommentInfo to MappedCommentLocation in both types.ts and processor.js. Also rebased on main and addressed all other review feedback.

@andreahlert
Copy link
Author

@lumirlumir Rebased on main, all feedback addressed. Tests and lint are passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Implementing

Development

Successfully merging this pull request may close these issues.

eslint --report-unused-disable-directives does not work with this plugin

6 participants