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

Escape strings containing font glyphs (Nerd Font compatibility) #343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JanDeDobbeleer
Copy link
Sponsor

@JanDeDobbeleer JanDeDobbeleer commented Feb 10, 2023

This fixes an issue with Nerd Font glyphs (and others, but this is my use-case) not being recognised resulting in a non-quoted string, which in YAML makes them literal text rather than icons. This breaks config exports and migrations for oh-my-posh.

Example:

map[string]string{"v": "\ue0b6"},

Is encoded to YAML as:

v: \ue0b6

But that should be:

v: "\ue0b6"

The isEmoticon function is the same as the one we use for oh-my-posh which has worked without issues so far.

@JanDeDobbeleer
Copy link
Sponsor Author

@goccy are you still maintaining this? I don't mind using my fork, but it would be amazing to get a conversation started around this.

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2023

Codecov Report

Merging #343 (5f846f9) into master (31fe1ba) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #343   +/-   ##
=======================================
  Coverage   76.02%   76.02%           
=======================================
  Files          13       13           
  Lines        4692     4692           
=======================================
  Hits         3567     3567           
  Misses        866      866           
  Partials      259      259           

Copy link
Owner

@goccy goccy 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 for your contribution !! I've some commented.

token/token.go Outdated Show resolved Hide resolved
decode_test.go Show resolved Hide resolved
decode.go Outdated Show resolved Hide resolved
decode.go Outdated Show resolved Hide resolved
scanner/scanner.go Outdated Show resolved Hide resolved
scanner/scanner.go Outdated Show resolved Hide resolved
token/token.go Outdated Show resolved Hide resolved
@JanDeDobbeleer
Copy link
Sponsor Author

JanDeDobbeleer commented Oct 12, 2023

@goccy only relevant changes remaining. And sorry it took me THIS long to address this again. Really bad behaviour....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants