Skip to content

Comments

refactor: correct no-reversed-media-syntax selectors#508

Merged
nzakas merged 2 commits intoeslint:mainfrom
TKDev7:refactor-selector
Aug 19, 2025
Merged

refactor: correct no-reversed-media-syntax selectors#508
nzakas merged 2 commits intoeslint:mainfrom
TKDev7:refactor-selector

Conversation

@TKDev7
Copy link
Contributor

@TKDev7 TKDev7 commented Aug 11, 2025

Prerequisites checklist

Example Code

# Heading with `inline` code

Paragraph with `(label)[url]` reversed syntax.

Current Behavior

The selector syntax: "heading html,inlineCode" is interpreted as:

  • html inside heading
  • OR any inlineCode node anywhere in the document

While in practice this behaves as intended since inlineCode can only appear inside headings, paragraphs, or table cells, the syntax can be misread and is less explicit about its scoping.

Expected Behavior

Selectors should clearly indicate that both html and inlineCode are matched only when nested within the relevant container node.

What is the purpose of this pull request?

This PR fixes selector patterns to make their scope explicit and easier to read, replacing comma-separated selectors with :matches() expressions.

What changes did you make? (Give an overview)

  • Replaced comma-separated selectors such as "heading html,inlineCode" with :matches() expressions like "heading :matches(html, inlineCode)"

Related Issues

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

@lumirlumir
Copy link
Member

lumirlumir commented Aug 12, 2025

Please create an issue before working on a fix so the team can align on the direction we want to take.

As far as I know, @snitin315 has been asking you to follow our contributing guidelines here: #500 (review), #480 (comment), eslint/css#171 (comment), eslint/rewrite#233 (comment)

I hope you'll follow our guidance.

Edit: Also, if you plan to submit a fix directly, please use our bug issue template so we can more clearly identify the problem.


I agree that this change could save some calculations, but I don't think it's a bug fix. Do you have any examples where this change is actually fixing false negatives or positives?

Since the inlineCode node can only be placed in heading, paragraph, and tableCell nodes, I'm not sure this change is really fixing a bug. It seems more like a code refactor with a minor performance improvement.

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Aug 12, 2025
@lumirlumir lumirlumir moved this from Needs Triage to Evaluating in Triage Aug 12, 2025
@TKDev7
Copy link
Contributor Author

TKDev7 commented Aug 12, 2025

@lumirlumir Could you please clarify why this PR doesn’t qualify for the exception criteria? I tried to describe the issue clearly in the PR.

  • If there isn’t an existing issue, then open one to describe the change you want to make. Use the appropriate issue template.
    • Exceptions: Small changes, such as bug fixes, documentation updates, or package upgrades don’t require an issue. Make sure the pull request description explains clearly what it is you’re doing and why.

@nzakas
Copy link
Member

nzakas commented Aug 13, 2025

I think there's been some confusion lately over what does and doesn't require an issue. Bug fixes typically do not require an issue, but we do want the PR description to contain the same information as a bug report. So we're looking for a clear description of the problem, reproduction steps, example code, current behavior, and expected behavior.

@TKDev7 if you can please update the description of the PR with this info, it would be very helpful.

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.

LGTM, Would like another review before merging.

I'm changing the type of the PR title to refactor, since this is not a user-facing bug fix.

@lumirlumir lumirlumir changed the title fix: correct no-reversed-media-syntax selectors refactor: correct no-reversed-media-syntax selectors Aug 15, 2025
@lumirlumir lumirlumir removed the bug label Aug 15, 2025
@lumirlumir lumirlumir moved this from Evaluating to Second Review Needed in Triage Aug 15, 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

@nzakas nzakas merged commit 737501b into eslint:main Aug 19, 2025
23 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants