search

cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

Use `lipgloss.Width` to calculate `DisplayWidth` #159

Closed maaslalani closed 2 months ago

maaslalani commented 2 months ago

Fixes https://github.com/cli/cli/issues/9018

Uses lipgloss.Width to calculate DisplayWidth which correctly aligns emojis.

maaslalani commented 2 months ago
image
maaslalani commented 2 months ago

@williammartin Had to switch to lipgloss.Width to account for ANSI sequences since uniseg doesn't strip ANSI before calculating width (one of the test cases included ANSI sequences), but lipgloss does then uses uniseg.StringWidth under the hood.

Added the multi-byte emoji test case as well!

williammartin commented 2 months ago

Thanks. If we're expecting DisplayWidth to handle ANSI escape sequences then let's add a test for that as well.

I suppose for someone out there this could be a breaking change but I think it's edge case enough that I'd rather just accept it and then people could fix any hacky workarounds they might have had.

maaslalani commented 2 months ago

Yep, there is already a test case for that! The tests were failing when I simply had uniseg.StringWidth because of the ANSI test case. Which is why I needed to swap out for lipgloss to pass all tests cases.

maaslalani commented 2 months ago

Test case for ignoring ANSI sequences when calculating width is here: https://github.com/cli/go-gh/blob/trunk/pkg/text/text_test.go#L282-L287 (fails when just using uniseg.StringWidth, passes when using lipgloss.Width)

williammartin commented 2 months ago

Well, that's testing the public API of PadRight but it's just an implementation detail that PadRight uses DisplayWidth. Refactoring PadRight would then lose all our confidence that DisplayWidth handles ANSI codes. I want to both ensure that DisplayWidth enforces this behaviour, and I want it to be communicated to future maintainers in the tests because I had no idea that it was expected when you first opened the PR.

maaslalani commented 2 months ago

Ah you're totally right, there is also a test case directly for color codes in DisplayWidth as well:

https://github.com/cli/go-gh/blob/5840c447a66255f2822dfc9be315be0f9ff1d6d2/pkg/text/text_test.go#L360-L363

Happy to add more test cases though if you want / prefer!

williammartin commented 2 months ago

Haha sorry I should have also looked closer. I think I got red herring-ed by your link because I assumed if it existed already you would definitely have linked it 😅

I can confirm that test fails just using uniseg.StringWidth:

--- FAIL: TestDisplayWidth (0.00s)
    --- FAIL: TestDisplayWidth/color_codes (0.00s)
        /Users/williammartin/workspace/go-gh/pkg/text/text_test.go:388: 
                Error Trace:    text_test.go:388
                Error:          Not equal: 
                                expected: 3
                                actual  : 12
                Test:           TestDisplayWidth/color_codes

No need for extra tests, this looks great to me, once I pull it into cli/cli and play with it, cheers.

maaslalani commented 2 months ago

I think I got red herring-ed by your link because I assumed if it existed already you would definitely have linked it 😅

Haha all good! I kind of missed that test case the first time as well and definitely should have linked to that!

williammartin commented 2 months ago

Before

image

(wow such codepoints: https://ardislu.dev/biggest-unicode-grapheme-cluster)

After

image