-
-
Notifications
You must be signed in to change notification settings - Fork 429
Add support for disabling rules via HTML comments #1767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Introduces parsing of htmlhint-disable and htmlhint-enable comments to selectively disable rules or all rules on specific lines. Updates the Reporter to respect these disable comments, preventing messages for disabled rules. Documentation and tests updated to reflect new functionality.
| }> = [] | ||
|
|
||
| let match: RegExpExecArray | null | ||
| while ((match = regComment.exec(html)) !== null) { |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
library input
This
regular expression
library input
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1767 +/- ##
===========================================
+ Coverage 96.99% 100.00% +3.00%
===========================================
Files 2 1 -1
Lines 1628 1 -1627
Branches 335 0 -335
===========================================
- Hits 1579 1 -1578
+ Misses 49 0 -49 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a great new feature for disabling rules via HTML comments, similar to other linters. The implementation is solid, covering block-level, next-line, all-rules, and specific-rules disabling. The core logic is well-contained in the new parseDisableComments method, and the reporter is updated to respect the disabled rules. The documentation and tests are also updated accordingly.
I've made a couple of suggestions:
- One for improving the performance and readability of the comment parsing logic in
src/core/core.ts. - Another to clarify a misleading comment in one of the new tests in
test/core.spec.js.
Overall, this is a well-executed feature.
| private parseDisableComments(html: string): DisabledRulesMap { | ||
| const disabledRulesMap: DisabledRulesMap = {} | ||
| const lines = html.split(/\r?\n/) | ||
| const regComment = | ||
| /<!--\s*htmlhint-(disable|enable)(?:-next-line)?(?:\s+([^\r\n]+?))?\s*-->/gi | ||
|
|
||
| // Find all disable/enable comments and their positions | ||
| const comments: Array<{ | ||
| line: number | ||
| command: string | ||
| isNextLine: boolean | ||
| rulesStr?: string | ||
| }> = [] | ||
|
|
||
| let match: RegExpExecArray | null | ||
| while ((match = regComment.exec(html)) !== null) { | ||
| // Calculate line number from match position | ||
| const beforeMatch = html.substring(0, match.index) | ||
| const lineNumber = beforeMatch.split(/\r?\n/).length | ||
| const command = match[1].toLowerCase() | ||
| const isNextLine = match[0].includes('-next-line') | ||
| const rulesStr = match[2]?.trim() | ||
|
|
||
| comments.push({ | ||
| line: lineNumber, | ||
| command, | ||
| isNextLine, | ||
| rulesStr, | ||
| }) | ||
| } | ||
|
|
||
| // Process comments in order | ||
| let currentDisabledRules: Set<string> | null = null | ||
| let isAllDisabled = false | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = i + 1 | ||
|
|
||
| // Check if there's a comment on this line | ||
| const commentOnLine = comments.find((c) => c.line === line) | ||
| if (commentOnLine) { | ||
| if (commentOnLine.command === 'disable') { | ||
| if (commentOnLine.isNextLine) { | ||
| // htmlhint-disable-next-line | ||
| const nextLine = line + 1 | ||
| if (commentOnLine.rulesStr) { | ||
| // Specific rules disabled | ||
| const rules = commentOnLine.rulesStr | ||
| .split(/\s+/) | ||
| .filter((r) => r.length > 0) | ||
| if (!disabledRulesMap[nextLine]) { | ||
| disabledRulesMap[nextLine] = {} | ||
| } | ||
| if (!disabledRulesMap[nextLine].rules) { | ||
| disabledRulesMap[nextLine].rules = new Set() | ||
| } | ||
| rules.forEach((r) => disabledRulesMap[nextLine].rules!.add(r)) | ||
| } else { | ||
| // All rules disabled | ||
| if (!disabledRulesMap[nextLine]) { | ||
| disabledRulesMap[nextLine] = {} | ||
| } | ||
| disabledRulesMap[nextLine].all = true | ||
| } | ||
| } else { | ||
| // htmlhint-disable | ||
| if (commentOnLine.rulesStr) { | ||
| // Specific rules disabled | ||
| const rules = commentOnLine.rulesStr | ||
| .split(/\s+/) | ||
| .filter((r) => r.length > 0) | ||
| currentDisabledRules = new Set(rules) | ||
| isAllDisabled = false | ||
| } else { | ||
| // All rules disabled | ||
| currentDisabledRules = null | ||
| isAllDisabled = true | ||
| } | ||
| } | ||
| } else if (commentOnLine.command === 'enable') { | ||
| // htmlhint-enable | ||
| currentDisabledRules = null | ||
| isAllDisabled = false | ||
| } | ||
| } | ||
|
|
||
| // Apply current disable state to this line (if not already set by next-line) | ||
| if (currentDisabledRules !== null || isAllDisabled) { | ||
| if (!disabledRulesMap[line]) { | ||
| disabledRulesMap[line] = {} | ||
| } | ||
| // Don't override if already set by next-line comment | ||
| if (isAllDisabled && disabledRulesMap[line].all !== true) { | ||
| disabledRulesMap[line].all = true | ||
| } else if (currentDisabledRules) { | ||
| if (!disabledRulesMap[line].rules) { | ||
| disabledRulesMap[line].rules = new Set() | ||
| } | ||
| currentDisabledRules.forEach((r) => | ||
| disabledRulesMap[line].rules!.add(r) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return disabledRulesMap | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is a great addition! I have a couple of suggestions to improve its performance and readability.
-
Performance: The current implementation uses
comments.find()inside a loop that iterates over every line of the file. This results in a time complexity of approximately O(lines × comments), which could be slow for large files. You could optimize this by first grouping comments by line number into aMap, reducing the complexity to O(lines + comments). This also makes the code more robust by handling multiple disable comments on the same line, whichfind()does not. -
Readability & Duplication: There are a few places where the code could be made more concise and safer:
- The logic to parse rules from
rulesStris repeated. This could be extracted into a helper function. - Object initialization like
if (!disabledRulesMap[nextLine]) { disabledRulesMap[nextLine] = {} }is also repeated. - The non-null assertion operator (
!) is used (e.g.,rules!.add(r)). While it seems safe here, it's good practice to avoid it when possible.
- The logic to parse rules from
Here's a sketch of a potential refactoring for the performance part:
// First, group comments by line number
const commentsByLine = new Map<number, Array</* comment type */>>();
for (const comment of comments) {
if (!commentsByLine.has(comment.line)) {
commentsByLine.set(comment.line, []);
}
commentsByLine.get(comment.line)!.push(comment);
}
// Then, in your main loop
for (let i = 0; i < lines.length; i++) {
const line = i + 1;
const commentsOnLine = commentsByLine.get(line);
if (commentsOnLine) {
for (const commentOnLine of commentsOnLine) {
// ... process each comment on the line
}
}
// ... rest of the logic
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Deploying htmlhint with
|
| Latest commit: |
b298cbc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d217d5d0.htmlhint.pages.dev |
| Branch Preview URL: | https://1227-comment-to-disableignor.htmlhint.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces support for disabling HTMLHint rules inline using HTML comments, allowing developers to selectively suppress linting errors for specific code sections.
Key changes:
- Adds
htmlhint-disable,htmlhint-enable, and-next-linevariant comment support for controlling rule enforcement - Implements parsing logic to track disabled rules per line and filters messages in the Reporter
- Provides comprehensive documentation with usage examples for all disable comment patterns
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/types.ts |
Adds DisabledRulesMap interface for tracking disabled rules by line number |
src/core/reporter.ts |
Implements filtering logic to skip messages for disabled rules before adding to results |
src/core/core.ts |
Adds parseDisableComments() method to parse disable/enable comments and build the disabled rules map |
dist/core/reporter.js |
Compiled JavaScript output for reporter changes |
dist/core/core.js |
Compiled JavaScript output for core changes |
test/core.spec.js |
Adds comprehensive test suite covering various disable/enable comment scenarios |
website/src/content/docs/configuration.md |
Documents the new inline rule disabling feature with examples |
website/src/content/docs/getting-started.mdx |
Adds two rules to the default configuration example (unrelated change) |
AGENTS.md |
New file with AI agent coding guidelines (unrelated change) |
| <div class="bar">Ipsum</div>` | ||
| const messages = HTMLHint.verify(code, { | ||
| 'attr-lowercase': true, | ||
| }) | ||
| // Line 2 should be disabled (no errors), line 3 is valid (no errors) | ||
| expect(messages.length).toBe(0) |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expectation is incorrect. Line 3 (<div class="bar">Ipsum</div>) uses lowercase class, which is valid and should not trigger the attr-lowercase rule. The test comment states "line 3 should have errors", but this is inaccurate. The test should expect 0 messages, not check for errors on line 3, OR the test should be updated to use an uppercase attribute on line 3 (e.g., <div CLASS="bar">Ipsum</div>) to properly test that line 3 is not disabled.
| <div class="bar">Ipsum</div>` | |
| const messages = HTMLHint.verify(code, { | |
| 'attr-lowercase': true, | |
| }) | |
| // Line 2 should be disabled (no errors), line 3 is valid (no errors) | |
| expect(messages.length).toBe(0) | |
| <div CLASS="bar">Ipsum</div>` | |
| const messages = HTMLHint.verify(code, { | |
| 'attr-lowercase': true, | |
| }) | |
| // Line 2 should be disabled (no errors), line 3 should have errors | |
| expect(messages.length).toBe(1) | |
| expect(messages[0].line).toBe(3) | |
| expect(messages[0].rule.id).toBe('attr-lowercase') |
| for (let i = 0; i < lines.length; i++) { | ||
| const line = i + 1 | ||
|
|
||
| // Check if there's a comment on this line | ||
| const commentOnLine = comments.find((c) => c.line === line) | ||
| if (commentOnLine) { | ||
| if (commentOnLine.command === 'disable') { | ||
| if (commentOnLine.isNextLine) { | ||
| // htmlhint-disable-next-line | ||
| const nextLine = line + 1 | ||
| if (commentOnLine.rulesStr) { | ||
| // Specific rules disabled | ||
| const rules = commentOnLine.rulesStr | ||
| .split(/\s+/) | ||
| .filter((r) => r.length > 0) | ||
| if (!disabledRulesMap[nextLine]) { | ||
| disabledRulesMap[nextLine] = {} | ||
| } | ||
| if (!disabledRulesMap[nextLine].rules) { | ||
| disabledRulesMap[nextLine].rules = new Set() | ||
| } | ||
| rules.forEach((r) => disabledRulesMap[nextLine].rules!.add(r)) | ||
| } else { | ||
| // All rules disabled | ||
| if (!disabledRulesMap[nextLine]) { | ||
| disabledRulesMap[nextLine] = {} | ||
| } | ||
| disabledRulesMap[nextLine].all = true | ||
| } | ||
| } else { | ||
| // htmlhint-disable | ||
| if (commentOnLine.rulesStr) { | ||
| // Specific rules disabled | ||
| const rules = commentOnLine.rulesStr | ||
| .split(/\s+/) | ||
| .filter((r) => r.length > 0) | ||
| currentDisabledRules = new Set(rules) | ||
| isAllDisabled = false | ||
| } else { | ||
| // All rules disabled | ||
| currentDisabledRules = null | ||
| isAllDisabled = true | ||
| } | ||
| } | ||
| } else if (commentOnLine.command === 'enable') { | ||
| // htmlhint-enable | ||
| currentDisabledRules = null | ||
| isAllDisabled = false | ||
| } | ||
| } | ||
|
|
||
| // Apply current disable state to this line (if not already set by next-line) | ||
| if (currentDisabledRules !== null || isAllDisabled) { | ||
| if (!disabledRulesMap[line]) { | ||
| disabledRulesMap[line] = {} | ||
| } | ||
| // Don't override if already set by next-line comment | ||
| if (isAllDisabled && disabledRulesMap[line].all !== true) { | ||
| disabledRulesMap[line].all = true | ||
| } else if (currentDisabledRules) { | ||
| if (!disabledRulesMap[line].rules) { | ||
| disabledRulesMap[line].rules = new Set() | ||
| } | ||
| currentDisabledRules.forEach((r) => | ||
| disabledRulesMap[line].rules!.add(r) | ||
| ) | ||
| } | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The line containing the htmlhint-disable comment itself gets marked as disabled. When processing a line with a htmlhint-disable comment (not -next-line), the code first updates the disable state (lines 147-159), then immediately applies that state to the current line (lines 169-184).
For example, with:
Line 1: <!-- htmlhint-disable -->
Line 2: <div CLASS="foo">Lorem</div>
The code will disable both line 1 (the comment line) and line 2. This is likely unintended - typically, disable comments should only affect subsequent lines, not the line they appear on.
Fix: The application of disabled state (lines 169-184) should occur BEFORE processing the comment on that line, or the logic should skip applying the state when a htmlhint-disable (non-next-line) comment is found on the current line.
| "h1-require": true, | ||
| "html-lang-require": true, | ||
| "id-unique": true, | ||
| "input-requires-label": true, |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] These rule additions to the default configuration example appear to be unrelated to the disable/enable comments feature. While they are valid rules, they should be in a separate PR focused on documentation updates rather than mixed with the disable comments feature implementation.
| "input-requires-label": true, |
| <!-- htmlhint-enable --> | ||
| <div class="baz">Dolor</div> | ||
| ``` | ||
|
|
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation: The documentation doesn't clarify that htmlhint-enable re-enables ALL rules, regardless of which specific rules were disabled. For example, if you disable two specific rules with <!-- htmlhint-disable rule1 rule2 -->, using <!-- htmlhint-enable --> will re-enable all rules, not just rule1 and rule2. Additionally, htmlhint-enable does not support specifying specific rules to re-enable (e.g., <!-- htmlhint-enable rule1 --> is not supported). This limitation should be clearly documented to set proper user expectations.
| > **Note:** `<!-- htmlhint-enable -->` will re-enable *all* rules, not just those previously disabled. Specifying particular rules to re-enable (e.g., `<!-- htmlhint-enable rule1 -->`) is not supported. |
| const line = i + 1 | ||
|
|
||
| // Check if there's a comment on this line | ||
| const commentOnLine = comments.find((c) => c.line === line) |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance issue: The code uses comments.find((c) => c.line === line) inside a loop over all lines (line 121), resulting in O(n*m) complexity where n is the number of lines and m is the number of comments. For large files with many comments, this could be slow.
Consider optimizing by pre-processing comments into a Map indexed by line number:
const commentsByLine = new Map<number, typeof comments[0]>()
comments.forEach(c => commentsByLine.set(c.line, c))
// Then use: const commentOnLine = commentsByLine.get(line)This would reduce complexity to O(n+m).
Introduces parsing of htmlhint-disable and htmlhint-enable comments to selectively disable rules or all rules on specific lines. Updates the Reporter to respect these disable comments, preventing messages for disabled rules. Documentation and tests updated to reflect new functionality.