fix: report messages for configuration comments correctly#618
fix: report messages for configuration comments correctly#618andreahlert wants to merge 3 commits intoeslint:mainfrom
Conversation
31d9362 to
dcccf5a
Compare
|
Thanks for the review, @Ari4ka! |
|
@andreahlert thanks, I'll take a look when I'm able. Did you use AI to generate this PR? |
|
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! |
|
Thank you for submitting a PR for this. It is a really good starting for fixing this. |
|
Hi — there's a CI failure. If you have a moment, could you take a look? |
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
Done! Ready for final review. |
|
|
||
| export interface BlockBase { | ||
| baseIndentText: string; | ||
| comments: string[]; |
There was a problem hiding this comment.
The comments property should be removed as it is no longer set.
There was a problem hiding this comment.
Can you double-check this? It seems like comments is still used.
There was a problem hiding this comment.
It seems the current implementation still uses the comments property. Otherwise it throws a type error.
There was a problem hiding this comment.
Confirmed, comments is still used in getBlockRangeMap, block.comments.join for assembling linted text, and leadingCommentLines calculation. Kept it as-is.
|
Sorry for the delay. Other than the small things I mentioned, the changes LGTM. |
nzakas
left a comment
There was a problem hiding this comment.
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[]; |
There was a problem hiding this comment.
Can you double-check this? It seems like comments is still used.
There was a problem hiding this comment.
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[]; |
There was a problem hiding this comment.
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>
e4ed80c to
aaa9b03
Compare
|
@nzakas Renamed |
|
@lumirlumir Rebased on main, all feedback addressed. Tests and lint are passing. |
Summary
Previously, ESLint messages that pointed to configuration comments (such as unused disable directive warnings) were being silently dropped because the
adjustMessagefunction returnednullfor any message pointing to lines before the actual code content.This change:
This fixes the issue where
--report-unused-disable-directiveswould not work with the markdown processor, as those warnings were being filtered out.Before
After
Test plan
reportUnusedDisableDirectiveswith tests for single-line and multi-line HTML commentsFixes #115