Skip to content

Conversation

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 3, 2025

Prerequisites checklist

What is the purpose of this pull request?

Fixes report location in the no-multiple-h1 and require-alt-text rules in cases where a preceding HTML comment has characters consisting of Unicode surrogate pairs.

image

What changes did you make? (Give an overview)

Updated utils.stripHtmlComments to replace each code unit (instead of code point) with a space. This preserves offsets and locations of characters outside HTML comments.

Related Issues

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

@eslint-github-bot eslint-github-bot bot added the bug label Oct 3, 2025
@eslintbot eslintbot added this to Triage Oct 3, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Oct 3, 2025
@mdjermanovic
Copy link
Member Author

I'm not sure how 👍🚀 becomes \u{1f44d}\u{1f680} in Bun. That causes incorrect locations in tests.

@mdjermanovic
Copy link
Member Author

Seems that dedent`` is causing this. Not sure why, but I'll update the tests.

@JoshuaKGoldberg
Copy link
Contributor

dmnd/dedent#101, perhaps?

@lumirlumir lumirlumir self-requested a review October 3, 2025 16:35
@mdjermanovic
Copy link
Member Author

dmnd/dedent#101, perhaps?

Yes, looks like that is exactly the same problem. I've just updated the added tests to not use dedent, and they're now passing in Bun.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM 👍. Leaving open for a second review because this is tricky code unit logic.

@JoshuaKGoldberg JoshuaKGoldberg moved this from Needs Triage to Second Review Needed in Triage Oct 3, 2025
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, thanks. Good catch!

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.

4 participants