Skip to content

Conversation

@vvoland
Copy link
Collaborator

@vvoland vvoland commented Jan 14, 2025

Something I noticed a while ago when implementing the docker images --tree but forgot about posting:

Align with upstream implementation: https://github.com/golang/go/blob/6da16013ba4444e0d71540f68279f0283a92d05d/src/text/tabwriter/tabwriter.go#L417

This allows us to drop the dependency on mattn/go-runewidth package.

Also, runewidth doesn't actually provide correct results in some cases.
For comparison:

utf8.RuneCountInString("👍") = 1
runewidth.StringWidth("👍") = 2

In this case, the "thumbs up" emoji takes one cell, so its width should be equal to 1.
Actually, the correct value is 2 as it actually takes two cells!

Align with upstream implementation: https://github.com/golang/go/blob/6da16013ba4444e0d71540f68279f0283a92d05d/src/text/tabwriter/tabwriter.go#L417

This allows us to drop the dependency on `mattn/go-runewidth` package.

Also, `runewidth` doesn't actually provide correct results in some
cases.
For comparison:
```
utf8.RuneCountInString("👍") = 1
runewidth.StringWidth("👍") = 2
```

In this case, the "thumbs up" emoji takes one cell, so its width should
be equal to `1`.

Signed-off-by: Paweł Gronowski <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.44%. Comparing base (25bdd5c) to head (82f7307).

❗ There is a different number of reports uploaded between BASE (25bdd5c) and HEAD (82f7307). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (25bdd5c) HEAD (82f7307)
21 16
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5749       +/-   ##
===========================================
- Coverage   59.51%   44.44%   -15.08%     
===========================================
  Files         346        6      -340     
  Lines       29376      261    -29115     
===========================================
- Hits        17483      116    -17367     
+ Misses      10923      139    -10784     
+ Partials      970        6      -964     

@thaJeztah
Copy link
Member

Ah! Looks liike this doesn't work though 🤔

Wondering if rune count != width. The problem this package was solving is for both wide characters, but I think also characters that are multiple runes, but take up 1 column.

@vvoland
Copy link
Collaborator Author

vvoland commented Jan 14, 2025

Right! So in this case, is the upstream Go implementation wrong now? https://github.com/golang/go/blob/6da16013ba4444e0d71540f68279f0283a92d05d/src/text/tabwriter/tabwriter.go#L417 😅

@vvoland
Copy link
Collaborator Author

vvoland commented Jan 14, 2025

Hmm right, so looks like we explicitly forked tabwriter to change that: #3568

Let me close this PR then 😅

TIL

@vvoland vvoland closed this Jan 14, 2025
@vvoland vvoland deleted the remove-runewidth branch January 14, 2025 15:17
@thaJeztah
Copy link
Member

Ah, yes, had to read back the other issues, but I think the go implementation has bugs, but they closed it as WONTFIX; golang/go#8273 (comment)

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.

3 participants