Wrap last column to terminal width in CLI table formatter#11931
Wrap last column to terminal width in CLI table formatter#11931zachcasper wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI table formatter to wrap long last-column values to the terminal width, improving readability for tables with long descriptions while preserving unwrapped output when terminal width is unknown.
Changes:
- Replaced
text/tabwriterusage with a custom table renderer. - Added last-column word wrapping, continuation-line padding, and related tests.
- Promoted
golang.org/x/termto a direct dependency.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/cli/output/table.go |
Implements terminal-width detection, custom column sizing, padding, and wrapping helpers. |
pkg/cli/output/table_test.go |
Adds tests for wrapping behavior, unknown-width output, long words, Unicode, and whitespace preservation. |
go.mod |
Marks golang.org/x/term as a direct dependency. |
Address Copilot review feedback on radius-project#11931: replace utf8.RuneCountInString with runewidth.StringWidth so that wide characters (CJK, emoji) and zero-width combining marks are accounted for correctly in column alignment and last-column wrapping. Long unbreakable words are now split at display-column boundaries rather than rune-count boundaries. - pkg/cli/output/table.go: switch padRight and wordWrap (and the slot/width computation) to use runewidth.StringWidth and runewidth.RuneWidth. - pkg/cli/output/table_test.go: add Test_Table_WrapsWideCharactersByDisplayWidth exercising CJK, and update the Unicode wrap expectation to reflect that 🚀 is two display columns wide. - go.mod: promote github.com/mattn/go-runewidth to a direct dependency.
Address Copilot review feedback on radius-project#11931: replace utf8.RuneCountInString with runewidth.StringWidth so that wide characters (CJK, emoji) and zero-width combining marks are accounted for correctly in column alignment and last-column wrapping. Long unbreakable words are now split at display-column boundaries rather than rune-count boundaries. - pkg/cli/output/table.go: switch padRight and wordWrap (and the slot/width computation) to use runewidth.StringWidth and runewidth.RuneWidth. - pkg/cli/output/table_test.go: add Test_Table_WrapsWideCharactersByDisplayWidth exercising CJK, and update the Unicode wrap expectation to reflect that 🚀 is two display columns wide. - go.mod: promote github.com/mattn/go-runewidth to a direct dependency. Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11931 +/- ##
==========================================
+ Coverage 51.84% 51.92% +0.08%
==========================================
Files 729 729
Lines 46024 46127 +103
==========================================
+ Hits 23861 23953 +92
- Misses 19888 19896 +8
- Partials 2275 2278 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Address Copilot review feedback on radius-project#11931: replace utf8.RuneCountInString with runewidth.StringWidth so that wide characters (CJK, emoji) and zero-width combining marks are accounted for correctly in column alignment and last-column wrapping. Long unbreakable words are now split at display-column boundaries rather than rune-count boundaries. - pkg/cli/output/table.go: switch padRight and wordWrap (and the slot/width computation) to use runewidth.StringWidth and runewidth.RuneWidth. - pkg/cli/output/table_test.go: add Test_Table_WrapsWideCharactersByDisplayWidth exercising CJK, and update the Unicode wrap expectation to reflect that 🚀 is two display columns wide. - go.mod: promote github.com/mattn/go-runewidth to a direct dependency. Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Fixes golangci-lint ineffassign: currentWidth is reassigned a few lines later from the long-word chunking loop, so the intermediate reset to 0 was dead. Signed-off-by: Zach Casper <zachcasper@microsoft.com>
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
brooke-hamilton
left a comment
brooke-hamilton
left a comment
There was a problem hiding this comment.
Automated review consolidating findings from two reviewer passes. Two reviewers independently flagged the TableColumnMinWidth clamp as the primary functional bug; other comments cover API hygiene, test coverage gaps, and small dead-code/style items.
Top priorities
- The
available < TableColumnMinWidthclamp can still overflow the terminal — the very condition this PR is meant to fix. Format(the production entry point) is not exercised by any new test; all new tests bypass it viaformatWithWidth.- New tests do not use
t.Parallel()and could be consolidated into a single table-driven test.
PR title: Accurate and descriptive — no change suggested.
Contributor doc impact: None. Internal renderer change only; no CLI surface, build, or workflow changes.
| ) | ||
|
|
||
| // terminalWidthForWriter is overridable for testing. | ||
| var terminalWidthForWriter = func(w io.Writer) int { |
There was a problem hiding this comment.
Two concerns in this area:
- Dead exported constants above.
TableTabSize,TablePadCharacter, andTableFlagswere only used to configuretext/tabwriter. After this change they have no callers in the workspace. Because they are exported, leaving them dangling pollutes the package API. Please remove them, or add a doc comment explaining they are retained for backwards compatibility. terminalWidthForWriteras a mutable package-levelvar. It's a test seam, but none of the new tests actually swap it (they all callformatWithWidthdirectly), so the seam is currently unused. Either drop it and testFormatend-to-end with an*os.Filepipe, or keep it and add a test that overrides it witht.Cleanuprestoring the original. Also: width is only detected when the writer's concrete type is*os.File. Anything wrapping stdout (e.g., a color-enabling writer,bufio.Writer, a tee) silently disables wrapping. Please confirm the CLI's real stdout path is a raw*os.File; otherwise users on real terminals won't get the new behavior.
| // fits in the remaining space, and wrap longer cells onto continuation lines. | ||
| lastContentWidth := slots[numCols-1] - TablePadSize | ||
| if terminalWidth > 0 { | ||
| nonLastSum := 0 |
There was a problem hiding this comment.
(High priority — flagged by both reviewers.) When nonLastSum + TableColumnMinWidth > terminalWidth, the last column is forced to TableColumnMinWidth and the rendered line will exceed the terminal width — the very thing this PR fixes. For example, if non-last columns consume 75 cells of an 80-cell terminal, available becomes 5, this clamp raises it to 10, and every line ends up 85 cells wide.
Consider available = max(1, terminalWidth-nonLastSum) so wrapping still occurs in narrow terminals, or — if you intentionally prefer a readable minimum at the cost of overflow — add a comment documenting that fixed-column-driven overflow can still occur. Either way, please add a regression test where non-last columns leave fewer than TableColumnMinWidth cells for the last column.
| lastContentWidth = available | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Two minor cleanups:
if numCols > 0is dead:numCols == len(options.Columns)and the function returns early at the top whenlen(options.Columns) == 0. Remove the guard.cells[lastIdx] = wrappedmutates the caller'scellsslice. It's harmless today because each row is iterated once, but it's a non-obvious side effect on a closure parameter. Consider assigning into a localcolCellsvariable, or document the contract on the closure.
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
When width <= 0, this returns []string{s} without splitting on newlines. That is correct (callers have already split on \n), but the public contract isn't obvious from the signature — please add a one-line comment to that effect.
| space := runes[j:k] | ||
| spaceWidth := runewidth.StringWidth(string(space)) | ||
| i = k | ||
|
|
There was a problem hiding this comment.
Long-word splitter edge case: if width == 1 and a wide rune (display width 2) appears, the ci > chunkStart guard prevents emitting an empty chunk, so a one-rune chunk of display width 2 will be produced — exceeding width. This is unreachable today because available is clamped to TableColumnMinWidth == 10, but worth either an assertion comment or a wordWrap test with width == 1 to document the contract.
Also: ci here is a rune index because word is []rune, but the surrounding code uses []rune(s) conversions elsewhere — a one-line comment clarifying that ci is a rune index (not a byte index) would help future readers.
| }, | ||
| } | ||
|
|
||
| func Test_Table_WrapsLastColumnToTerminalWidth(t *testing.T) { |
There was a problem hiding this comment.
None of the new tests (Test_Table_WrapsLastColumnToTerminalWidth, Test_Table_NoWrapWhenTerminalWidthUnknown, Test_Table_WrapsLongUnbreakableWord, Test_Table_AlignsAndWrapsUnicode, Test_Table_PreservesInternalWhitespace, Test_Table_WrapsWideCharactersByDisplayWidth) call t.Parallel(). Per the repo's Go instructions, tests without shared state should run in parallel. All of these are independent and safe to parallelize.
Additionally, they share an identical setup pattern (build []wrappableInput, create formatter + buffer, call formatWithWidth, compare to expected string). Please consolidate into a single table-driven Test_Table_Wrapping(t *testing.T) with sub-tests using t.Run + t.Parallel(). This eliminates duplication and makes adding cases trivial.
| require.Equal(t, expected, buffer.String()) | ||
| } | ||
|
|
||
| func Test_Table_NoWrapWhenTerminalWidthUnknown(t *testing.T) { |
There was a problem hiding this comment.
Every new test bypasses Format and calls the unexported formatWithWidth. Please add at least one test that drives Format with a non-*os.File writer (a bytes.Buffer) to verify the "unknown width → no wrap" path through the production entry point. This is also the simplest way to lift the terminalWidthForWriter coverage hole flagged by Codecov.
| // terminal display width rather than rune count: wide characters (e.g. CJK ideographs) | ||
| // occupy two columns, so the column slot must grow accordingly and wrapping must occur | ||
| // before content exceeds the terminal width. | ||
| func Test_Table_WrapsWideCharactersByDisplayWidth(t *testing.T) { |
There was a problem hiding this comment.
Missing edge-case coverage worth adding:
- Single-column table (
numCols == 1) — exercises thelastContentWidthpath withnonLastSum == 0. - Cell containing literal
\ncombined with wrapping — verifies the doc claim that embedded newlines still expand into multiple rows even whenterminalWidth == 0. - Empty rows / empty last cell — verifies no spurious blank continuation lines.
- Heading longer than the available last-column width — current code will wrap the heading; either assert this behavior or add a special case so headings are never wrapped.
- Narrow terminal where non-last columns leave fewer than
TableColumnMinWidthcells for the last column — regression test for the overflow issue flagged ontable.go.
| require ( | ||
| github.com/mattn/go-runewidth v0.0.23 | ||
| golang.org/x/term v0.43.0 | ||
| ) |
There was a problem hiding this comment.
golang.org/x/term v0.43.0 and github.com/mattn/go-runewidth v0.0.23 are correctly promoted to direct dependencies and removed from the indirect block. The diff stat shows no go.sum change — please confirm go mod tidy was run and go.sum is consistent before merge.
Description
Fixes the misaligned/ugly CLI table output when a cell in the last column (typically
DESCRIPTION) is longer than the terminal width. The previoustext/tabwriter-based renderer let long lines overflow and wrap at the terminal boundary, breaking column alignment on continuation lines.This PR replaces the table renderer with a custom implementation that:
golang.org/x/term.Example
Before:
After:
Type of change
Fixes: #9756
Contributor checklist
Please verify that the PR meets the following requirements, where applicable:
eng/design-notes/in this repository, if new APIs are being introduced.