Skip to content

Conversation

@thecalamiity
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request?

This pull request fixes a bug in the no-important rule where !important inside CSS comments was incorrectly reported as a violation. The rule should only flag actual !important flags in declarations, not those appearing within comments.

What changes did you make? (Give an overview)

  • Updated the no-important rule to strip comments from the declaration text before searching for !important.
  • Added tests to verify that !important inside comments is ignored, and only real !important flags are reported.

Related Issues

#217 (comment)

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

Copy link
Contributor

Copilot AI left a 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 pull request fixes a bug in the CSS no-important rule where !important declarations inside CSS comments were incorrectly flagged as violations. The rule should only detect actual !important flags in CSS declarations, not those appearing within comment blocks.

  • Updated the regex pattern and comment handling logic to strip comments before checking for !important
  • Added comprehensive test cases covering various scenarios of !important in comments vs actual declarations
  • Ensures proper line/column positioning is maintained after comment removal

Reviewed Changes

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

File Description
src/rules/no-important.js Updated rule logic to strip comments before detecting !important patterns
tests/rules/no-important.test.js Added test cases for comment scenarios and edge cases with mixed comment/declaration patterns

return {
Declaration(node) {
if (node.important) {
const declarationText = context.sourceCode.getText(node);
Copy link

Copilot AI Aug 1, 2025

Choose a reason for hiding this comment

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

The nested replace operation to preserve newlines while replacing other characters with spaces is complex and may be unclear to future maintainers. Consider adding a comment explaining why newlines need to be preserved (likely for accurate line/column positioning).

Suggested change
const declarationText = context.sourceCode.getText(node);
const declarationText = context.sourceCode.getText(node);
// Replace all non-newline characters in comments with spaces, preserving newlines.
// This ensures that line/column offsets remain accurate for error reporting,
// since removing newlines would shift positions in the source text.

Copilot uses AI. Check for mistakes.
// Helpers
//-----------------------------------------------------------------------------

const importantPattern = /!\s*important/iu;
Copy link

Copilot AI Aug 1, 2025

Choose a reason for hiding this comment

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

The regex pattern for matching CSS comments should include a comment explaining its purpose and behavior, especially the non-greedy matching with *? which is crucial for handling multiple comments correctly.

Suggested change
const importantPattern = /!\s*important/iu;
const importantPattern = /!\s*important/iu;
/**
* Regex pattern to match CSS comments.
* Uses non-greedy matching (*?) to ensure that multiple comments are matched correctly,
* preventing the pattern from spanning across multiple comment blocks.
* Matches anything between /* and *\/, including newlines.
*/

Copilot uses AI. Check for mistakes.
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.

Thanks for taking a look at this. This is an edge case that is a bit more complex than it looks. To be accurate, we'd need to look for any number of comments. For example:

a {
   color: red ! /* !important */ /* another comment */ /* a third comment */important;
}

We also need to worry about line breaks inside of comments:

a {
   color: red ! /* !important */ /* another
 comment */ /* a third comment */important;
}

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Aug 1, 2025
@thecalamiity
Copy link
Contributor Author

I've added test cases for both of those scenarios, and they're working correctly.

@lumirlumir lumirlumir added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 2, 2025
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.

LGTM. Thanks!

@nzakas nzakas merged commit 3c6937a into eslint:main Aug 4, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion bug Something isn't working contributor pool

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants