Skip to content
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

fix: Emoji support #53 #125

Closed
wants to merge 1 commit into from

Conversation

myConsciousness
Copy link

Hi amazing developers,

Issue dart-lang/tools#1908 appears to have already been resolved in a previous pull request #123, but seems to be blocked due to CLA. So I have reopened a pull request on this issue instead.

Thank you.

@kevmoo kevmoo requested a review from natebosch April 18, 2022 16:00
@kevmoo
Copy link
Member

kevmoo commented Apr 18, 2022

@myConsciousness – could you drop the extra 0 that forces lines to change w/ no effect?

We also need tests here...

@natebosch
Copy link
Member

I don't understand why we are dropping the range 0x10000 to 0x10FFFF in order to add this one.

I'm also curios about @kevmoo comment. Were we relying on this to have valid output before? Will we need some other logic to be spec compliant?

cc @jonasfj

@myConsciousness
Copy link
Author

@kevmoo @natebosch - Thanks for your check!

I'm not familiar with the history of this library and would have liked to discuss the changes to the existing code made in #123 other than the emoji range.

I'm going to commit a patch with tests later :)

@jonasfj
Copy link
Member

jonasfj commented Apr 19, 2022

I think it might be wise to ask:

5.1. Character Set

To ensure readability, YAML streams use only the printable subset of the Unicode character set. The allowed character range explicitly excludes the C0 control block #x0-#x1F (except for TAB #x9, LF #xA, and CR #xD which are allowed), DEL #x7F, the C1 control block #x80-#x9F (except for NEL #x85 which is allowed), the surrogate block #xD800-#xDFFF, #xFFFE, and #xFFFF.

On input, a YAML processor must accept all Unicode characters except those explicitly excluded above.

@sigurdm sigurdm added the needs-info Additional information needed from the issue author label Sep 8, 2022
@alestiago
Copy link

What's the status of this? (cc: @sigurdm , @jonasfj , @kevmoo)

@github-actions github-actions bot removed the needs-info Additional information needed from the issue author label Jun 21, 2024
@kevmoo
Copy link
Member

kevmoo commented Aug 28, 2024

Didn't we fix this somewhere else?

@tamcy
Copy link
Contributor

tamcy commented Sep 22, 2024

@kevmoo Yes, fixed in #159. In master, not yet in release (is it ok to request one?).

@kevmoo
Copy link
Member

kevmoo commented Sep 23, 2024

Closing this out as it seems #159 fixed things.

@kevmoo kevmoo closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants