Skip to content

Calculate string width based on runes, not bytes#13

Merged
ryanuber merged 1 commit intoryanuber:masterfrom
marians:rune-width
Dec 20, 2016
Merged

Calculate string width based on runes, not bytes#13
ryanuber merged 1 commit intoryanuber:masterfrom
marians:rune-width

Conversation

@marians
Copy link
Contributor

@marians marians commented Dec 20, 2016

Golang len(“⌘”) returns 3, as it counts the number of bytes, not the number of visible characters. This change modifies length calculation to count runes, not bytes.

Using range to iterate over runes is proposed in this older blog post: https://blog.golang.org/strings

Golang `len(“⌘”)` returns 3, as it counts the number of bytes, not the
number of visible characters. This change modifies length calculation
to count runes, not bytes.

This also adds a test case TestColumnWidthCalculatorNonASCII
@ryanuber
Copy link
Owner

This makes sense, LGTM! Thanks @marians!

@ryanuber ryanuber merged commit 0fbbb3f into ryanuber:master Dec 20, 2016
@marians
Copy link
Contributor Author

marians commented Dec 21, 2016

Great, thanks for merging!

FYI: We have another fork (based on an older version) of columnize with a patch for handling colored text (ANSII escape codes) in https://github.com/giantswarm/columnize, which we use for our CLI https://github.com/giantswarm/gsctl . The implementation is a bit dirty, as we first remove ANSII codes for column width calculation, but then have to add to that width value again for the format strings. However, it seems to work.

If you are interested in merging this, I could prepare a PR for the latest columnize version.

@marians marians deleted the rune-width branch December 21, 2016 08:26
@ryanuber
Copy link
Owner

@marians I would love to support color codes. I think we may have to introduce an option for parsing them, though. This package is used in a few CLI's which can dump massive amounts of text, and we have had to optimize it once already (see #10), so regex parsing each line will become quite expensive. I actually checked out your branch and ran the benchmarks out of curiosity, and the difference goes from 2218254 ns/op to 11164945 ns/op on my machine (big difference on thousands of lines). It's unfortunate, but I still think the color code support will be worth it.

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.

2 participants