fix: report messages for configuration comments correctly#618
fix: report messages for configuration comments correctly#618andreahlert wants to merge 1 commit intoeslint:mainfrom
Conversation
|
|
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
andreahlert
force-pushed
the
fix/report-unused-disable-directives
branch
from
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! |
|
|
||
| This example enables the `alert` global variable, disables the `no-alert` rule, and configures the `quotes` rule to prefer single quotes: | ||
|
|
||
| <!-- eslint-skip --> |
There was a problem hiding this comment.
I think we should rather disable reporting unused disable directives by setting linterSettings: { reportUnusedDisableDirectives: false } } (and disable @eslint-community/eslint-comments-disable-enable-pair) in the override with the name "markdown/code-blocks/js".
There was a problem hiding this comment.
I’d keep the doc as-is for this PR. The goal here is to fix #115 so that --report-unused-disable-directives actually works with the plugin instead of dropping those warnings. Disabling the feature in the default config would avoid the bug but wouldn’t fix it. If the team prefers to turn it off by default in the markdown override, that could be a separate follow-up. WDYT?
| const commentText = wrapComment(commentInfo.text); | ||
| const commentLineCount = commentText.split("\n").length; | ||
|
|
||
| for (let i = 0; i < commentLineCount; i++) { |
There was a problem hiding this comment.
Question to the team: Do we want to support multiline ignore directives?
There was a problem hiding this comment.
I added multiline support so that when a directive is split across lines in the HTML comment, the mapping still points to the right place in the Markdown. If the team prefers to only support single-line directives for now, we can simplify and drop the multiline handling.
| if (commentInfo) { | ||
| return { | ||
| ...message, | ||
| line: commentInfo.position.start.line, |
There was a problem hiding this comment.
Question to the team: Is it okay that the precise location is lost?
For example when a disable directive has multiple rules but only one is unused, the location spans only the name of the unused rule instead of the whole directive.
There was a problem hiding this comment.
I'm having a hard time parsing your question. Are you saying that previously we spanned only the unused rule name and this change loses that? If so, I'd say we don't want to lose that.
Or are you saying we didn't span only the unused rule name and this PR does? Then I'd say this is the preferred approach.
There was a problem hiding this comment.
To clarify for @nzakas: before this PR those messages were dropped (no report at all). With this PR we report them but the position is the whole HTML comment (start line of the comment in the Markdown), not the exact column of the unused rule name inside the comment. So we’re not losing the previous “span only the unused rule name” behavior; we’re adding reporting with a coarser location.
If the team wants column-level precision (e.g. only the unused rule name) we’d need extra mapping from the linted code columns back to the original comment;
I can explore that in a follow-up if needed.
|
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? |
5 participants
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