fix: align check summary table for wide-character skill names#394
Open
greymoth-jp wants to merge 1 commit into
Open
fix: align check summary table for wide-character skill names#394greymoth-jp wants to merge 1 commit into
greymoth-jp wants to merge 1 commit into
Conversation
The skill summary table aligns columns by terminal display width (padRight uses runewidth.StringWidth), but the dynamic Skill column width was computed with utf8.RuneCountInString and truncateName cut by rune count. A wide character (CJK, fullwidth) is one rune but two display cells, so wide skill names overflowed the column and pushed every later column to the right. Measure the column and truncate by display width to match padRight.
|
@greymoth-jp please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes waza check summary table alignment when skill names contain wide characters (e.g., CJK/fullwidth and some emoji) by consistently measuring and truncating the dynamic “Skill” column using terminal display width via go-runewidth.
Changes:
- Compute the dynamic “Skill” column width with
runewidth.StringWidthinstead of rune count. - Truncate skill names with
runewidth.Truncateto keep display-cell widths within the column. - Add a regression test covering wide-character alignment in the printed summary table.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/waza/cmd_check.go | Switches name width measurement + truncation to terminal display width for correct alignment with wide characters. |
| cmd/waza/cmd_check_test.go | Adds a test asserting the summary table’s second column alignment holds for CJK skill names. |
| // Compute dynamic column width (in terminal display cells) from the longest | ||
| // skill name. Display width, rather than the rune count, is used so the | ||
| // column stays aligned with padRight for wide characters (CJK, emoji). | ||
| nameWidth := len("Skill") |
Comment on lines
+257
to
259
| if w := runewidth.StringWidth(n); w > nameWidth { | ||
| nameWidth = w | ||
| } |
Comment on lines
+341
to
+349
| // truncateName shortens a name so that its terminal display width is at most | ||
| // maxLen cells, appending "…" when truncation happens. Display width is used | ||
| // (rather than the rune count) so that wide characters such as CJK and emoji | ||
| // stay aligned with padRight in the summary table. | ||
| func truncateName(name string, maxLen int) string { | ||
| runes := []rune(name) | ||
| if len(runes) <= maxLen { | ||
| if runewidth.StringWidth(name) <= maxLen { | ||
| return name | ||
| } | ||
| return string(runes[:maxLen-1]) + "…" | ||
| return runewidth.Truncate(name, maxLen, "…") |
Comment on lines
+729
to
+735
| for _, line := range lines { | ||
| if !strings.Contains(line, "Medium-High") { | ||
| continue | ||
| } | ||
| col, _ := displayCol(line, "Medium-High") | ||
| assert.Equal(t, want, col, "second column misaligned for row %q", line) | ||
| } |
Member
|
Hi @greymoth-jp — thanks for the fix! This PR is ready to merge once the CLA is signed. Please reply on this PR with: (add |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
waza checkprints a summary table that lines its columns up by terminal display width:padRightpads withrunewidth.StringWidth, and the fixed columns are commented as "display columns ... for emoji-safe alignment".The dynamic "Skill" column does not follow that rule. Its width comes from
utf8.RuneCountInString, andtruncateNamecuts by rune count. For ASCII one rune is one cell, so it looks fine, but a wide character (CJK, fullwidth) is one rune and two display cells. A skill named for example日本語スキルcounts as 6 when the column width is chosen and then renders as 12 cells, so the column overflows and every column after it shifts to the right.Fix
Measure the column width and truncate with
runewidth.StringWidth/runewidth.Truncate, the same helperpadRightalready uses. ASCII output is unchanged:runewidth.Truncate("appinsights-instrumentation", 25, "…")returns the sameappinsights-instrumentat…that the existing test expects. Wide names now stay inside their column.Test
Added
TestPrintCheckSummaryTable_WideCharAlignment, which asserts that the second column starts at the same display offset on the header row and on a row whose skill name is CJK. It fails before the change (offset 14 vs 12) and passes after.go vet ./cmd/waza/and the existingcmd/wazatable tests stay green.