Skip to content

Recognize supplementary characters#913

Merged
xoofx merged 29 commits intoxoofx:masterfrom
tats-u:rune
Jan 12, 2026
Merged

Recognize supplementary characters#913
xoofx merged 29 commits intoxoofx:masterfrom
tats-u:rune

Conversation

@tats-u
Copy link
Contributor

@tats-u tats-u commented Nov 13, 2025

Fixes #860

@tats-u tats-u marked this pull request as draft December 28, 2025 10:00
@tats-u tats-u marked this pull request as ready for review December 31, 2025 10:33
@tats-u tats-u requested a review from MihaZupan December 31, 2025 10:35
@tats-u
Copy link
Contributor Author

tats-u commented Dec 31, 2025

I ran Unit Test in my VS 2026 with .NET 6/8/9 locally and all test cases succeeded.
Sorry for having submitted the half-baked PR.

@xoofx
Copy link
Owner

xoofx commented Dec 31, 2025

Thanks for the work! LGTM. Quite heavy with the Rune polyfill but we don't have much choices.

@MihaZupan anything other feedback?

@tats-u
Copy link
Contributor Author

tats-u commented Dec 31, 2025

@tats-u
Copy link
Contributor Author

tats-u commented Dec 31, 2025

If we adopt C# 14, we can use Extension Members instead of creating the UnicodeUtilitySupplement class.

@MihaZupan
Copy link
Collaborator

If we adopt C# 14, we can use Extension Members instead of creating the UnicodeUtilitySupplement class.

Could you get by with making the existing UnicodeUtlity file partial instead?

@tats-u
Copy link
Contributor Author

tats-u commented Jan 2, 2026

Could you get by with making the existing UnicodeUtlity file partial instead?

I moved the new members to the existing UnicodeUtility.cs. Should UnicodeUtility.cs also exist in the Polyfills directory with partial added to both class definitions?

@MihaZupan
Copy link
Collaborator

Should UnicodeUtility.cs also exist in the Polyfills directory with partial added to both class definitions?

I don't think it's needed

@tats-u
Copy link
Contributor Author

tats-u commented Jan 2, 2026

I see. Let me know if we should move it from Helpers to the Polyfills directory.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 4, 2026

@MihaZupan I have finished correcting all sections where your additional responses were not required.

@tats-u tats-u requested a review from MihaZupan January 4, 2026 05:46
@tats-u tats-u requested a review from MihaZupan January 4, 2026 15:28
Copy link
Collaborator

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Thank you

tats-u and others added 3 commits January 5, 2026 23:29
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2026

image

After accepting your suggestions, some tests now fail.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2026

I managed to get all tests passed again.

@tats-u tats-u requested a review from MihaZupan January 8, 2026 15:21
// Advance to the next character, checking for a valid surrogate pair
char.IsHighSurrogate(Text[start++])
// Don't unconditionally increment `start` here. Check the surrogate code unit at `start` is a part of a valid surrogate pair first.
&& start <= End
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2026

I will need to merge master and fix the workflow.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2026

I hope the new workflow works.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 11, 2026

@MihaZupan I did all what I have to do now. Please let me know if I still have ones. The ball is in your court now. You can directly push commits here if necessary.

@MihaZupan
Copy link
Collaborator

I opened a PR against your fork with some cleanup tats-u#1

@tats-u
Copy link
Contributor Author

tats-u commented Jan 12, 2026

@MihaZupan Couldn't you directly push to my branch?

@MihaZupan
Copy link
Collaborator

Sure, but for the most part I try not to do that to avoid breaking contributors' workflows by messing up their branch. Pressing merge on the PR in your repo has the same effect.

@tats-u
Copy link
Contributor Author

tats-u commented Jan 12, 2026

Pressing merge on the PR in your repo has the same effect.

  • "Create a merge commit" → an extra commit will be added
  • "Squash and merge", "Rebase and merge" → the commit history is not compatible with your branch MihaZupan:rune-feedback

From now on, please feel free to add commits as needed, then just press “Squash and merge” at the end (all messy commits will be aggregated into a single commit).

@xoofx xoofx merged commit a89056d into xoofx:master Jan 12, 2026
3 checks passed
@tats-u
Copy link
Contributor Author

tats-u commented Jan 12, 2026

Thank you for your approval.

@xoofx
Copy link
Owner

xoofx commented Jan 12, 2026

Thank you for your approval.

Thank for your work! I will do a release later tonight.

@tats-u tats-u deleted the rune branch January 12, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recognize supplementary (non-BMP) punctuations around emphasis delimiter runs

3 participants