feat(format): add WidthStrategy interface for pluggable column width algorithms#569
feat(format): add WidthStrategy interface for pluggable column width algorithms#569
Conversation
…ms (#566) Extract the monolithic calculateOptimalWidth() behind a WidthStrategy interface, enabling runtime selection of column width algorithms via the CLI_WIDTH_STRATEGY system variable. Three strategies are implemented: - GREEDY_FREQUENCY (default): existing frequency-based greedy expansion - PROPORTIONAL: deficit-proportional allocation based on natural width - MARGINAL_COST: wrap-line minimization via max-heap (container/heap) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add enum-type tests for CLI_WIDTH_STRATEGY covering all values (GREEDY_FREQUENCY, PROPORTIONAL, MARGINAL_COST), case insensitivity, and invalid value rejection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the table formatting capabilities by introducing a flexible and extensible architecture for column width allocation. It abstracts the width calculation logic behind an interface, allowing different algorithms to be plugged in and selected at runtime. This change not only provides more control over table rendering but also lays the groundwork for future features like non-wrapping columns, improving the overall user experience and maintainability of the formatting subsystem. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the column width calculation logic by introducing a WidthStrategy interface. This is a great improvement for extensibility, allowing different width allocation algorithms to be plugged in and selected at runtime. The addition of Proportional and MarginalCost strategies alongside the existing GreedyFrequency logic is well-implemented, and the new CLI_WIDTH_STRATEGY system variable is correctly wired up. The accompanying tests and benchmarks are thorough.
My main feedback is on code organization within the new strategy files, where some utility functions are misplaced, impacting maintainability. Please see the specific comment for details.
Code Metrics Report📊 View detailed coverage report (available for 7 days)
Details | | main (f78f61c) | #569 (271ce71) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 70.0% | 70.5% | +0.4% |
| Files | 70 | 74 | +4 |
| Lines | 6800 | 6921 | +121 |
+ | Covered | 4765 | 4883 | +118 |
- | Code to Test Ratio | 1:1.2 | 1:1.2 | -0.1 |
| Code | 15840 | 16137 | +297 |
+ | Test | 20504 | 20769 | +265 |
+ | Test Execution Time | 2m9s | 1m27s | -42s |Code coverage of files in pull request scope (87.7% → 89.3%)
Reported by octocov |
Move wrapLinesForWidth and splitLines from width_strategy_proportional.go to width.go since they are shared by MarginalCostStrategy and tests. Replace custom indexOf with strings.IndexByte per review feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Performance improvement notes for future workAnalysis of Where
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed WidthStrategy interface to make column width calculation pluggable, which is a great improvement for extensibility. The implementation of the three strategies is clear and includes good test coverage and benchmarks. I've found one issue in the new ProportionalStrategy where leftover width might not be distributed, potentially leading to incorrect table formatting. My review includes a suggestion to fix this.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the column width calculation by introducing a WidthStrategy interface, allowing for pluggable algorithms. The existing frequency-based logic is moved into a GreedyFrequencyStrategy, and two new strategies, Proportional and MarginalCost, are added. A new system variable, CLI_WIDTH_STRATEGY, is introduced to select the algorithm at runtime. The changes are integrated into the formatting and system variable systems, and include new unit tests and benchmarks for the strategies. My review of the implementation did not identify any issues.
When all columns reach their natural width, leftover from integer division rounding was not distributed. Fall back to the first column to ensure the table always uses the full available width. Also add a NOTE comment to GreedyFrequencyStrategy documenting the same limitation inherited from the original calculateOptimalWidth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a well-executed refactoring that introduces a WidthStrategy interface to make the column width calculation algorithm pluggable. The existing logic is cleanly extracted into a GreedyFrequencyStrategy, and two new strategies (Proportional and MarginalCost) are added, providing more options for table formatting. The changes are well-tested with unit tests, comparison tests, and benchmarks.
I found one critical issue: a potential panic in the MarginalCostStrategy when handling a result with zero columns. I've left a comment with a suggested fix. Overall, this is a high-quality contribution that significantly improves the flexibility and design of the table formatting logic.
Pass verboseHeaders as the headers parameter to strategies instead of prepending them to rows. Strategies already include headers once in their data, so this ensures headers are counted exactly once. Update test expectation for verbose mode where the initial proportional allocation now correctly reflects verbose header display widths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strategies already include headers in their data, so passing headers as part of rows was double-counting them in benchmark measurements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a well-executed refactoring that introduces a WidthStrategy interface for pluggable column width algorithms. It successfully extracts the existing logic into a default strategy, adds two new advanced strategies, and fixes a header double-counting bug. The changes are well-structured and come with comprehensive tests and benchmarks. Upon further review, the previously identified potential panic in MarginalCostStrategy for zero columns is considered an over-engineered defensive check, as such scenarios are guaranteed not to occur in practice based on existing data source guarantees.
|
/gemini summary |
Summary of ChangesThis pull request refactors the table formatting subsystem to introduce a flexible and extensible architecture for column width allocation. By abstracting the width calculation logic behind an interface, it enables runtime selection of different algorithms, improving control over table rendering and setting the stage for future enhancements like non-wrapping columns. Highlights
Changelog
Activity
|
…olumns Add numCols > 0 check to prevent potential panic on currentWrapLines[0] when headers is empty. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring of the column width calculation logic. Introducing the WidthStrategy interface makes the system more modular and extensible, and the addition of new strategies like PROPORTIONAL and MARGINAL_COST provides valuable new formatting options. The bug fix for header double-counting is also a welcome improvement. I've found a couple of issues in the new strategy implementations that should be addressed.
| func(in Row) string { | ||
| return lo.Must(lo.Nth(in, columnIdx)).RawText() | ||
| }, |
There was a problem hiding this comment.
There's a potential panic here. The hiter.Map function iterates over all rows, including those from the rows slice. If any of these rows are shorter than len(headers), lo.Nth(in, columnIdx) will return an error for an out-of-bounds columnIdx, and lo.Must will cause a panic. The other new strategies (ProportionalStrategy and MarginalCostStrategy) correctly handle this by checking if the column index is within the row's bounds. A similar check should be added here for safety.
func(in Row) string {
if columnIdx < len(in) {
return in[columnIdx].RawText()
}
return ""
},| if remaining > 0 && totalDeficit > 0 { | ||
| distributed := 0 | ||
| for i := range numCols { | ||
| if deficits[i] > 0 { | ||
| share := remaining * deficits[i] / totalDeficit | ||
| // Don't exceed the natural width. | ||
| share = min(share, deficits[i]) | ||
| adjustedWidths[i] += share | ||
| distributed += share | ||
| } | ||
| } | ||
|
|
||
| // Assign leftover (from integer division rounding) to the column with | ||
| // the largest remaining deficit. If all columns have reached their | ||
| // natural width, fall back to the first column to ensure the full | ||
| // available width is always used. | ||
| leftover := remaining - distributed | ||
| if leftover > 0 && numCols > 0 { | ||
| remainingDeficits := make([]int, numCols) | ||
| for i := range numCols { | ||
| remainingDeficits[i] = naturalWidths[i] - adjustedWidths[i] | ||
| } | ||
| idx, _ := MaxWithIdx(0, slices.Values(remainingDeficits)) | ||
| if idx < 0 { | ||
| idx = 0 | ||
| } | ||
| adjustedWidths[idx] += leftover | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of ProportionalStrategy does not distribute any remaining width if totalDeficit is 0. This can happen if the initial header-based allocation is already sufficient for the natural width of all columns. As a result, the table may not use the full available screen width. The MarginalCostStrategy and the fallback logic for distributing leftover width suggest the intent is to always utilize the available space. The logic should be adjusted to handle this case, for example by distributing the remaining width to the column with the largest natural width.
if remaining > 0 {
distributed := 0
if totalDeficit > 0 {
for i := range numCols {
if deficits[i] > 0 {
share := remaining * deficits[i] / totalDeficit
// Don't exceed the natural width.
share = min(share, deficits[i])
adjustedWidths[i] += share
distributed += share
}
}
}
// Assign leftover (from integer division rounding or if totalDeficit was 0)
// to the column with the largest remaining deficit. If all columns have
// reached their natural width, fall back to the first column to ensure
// the full available width is always used.
leftover := remaining - distributed
if leftover > 0 && numCols > 0 {
remainingDeficits := make([]int, numCols)
for i := range numCols {
remainingDeficits[i] = naturalWidths[i] - adjustedWidths[i]
}
idx, _ := MaxWithIdx(0, slices.Values(remainingDeficits))
if idx < 0 {
idx = 0
}
adjustedWidths[idx] += leftover
}
}
Fix unresolved review issues from #569: GreedyFrequencyStrategy panic on short rows, ProportionalStrategy not distributing remaining width when totalDeficit is zero, and GreedyFrequencyStrategy not using full width when all columns fit. Also reduce allocations in transposedRows construction and adjustToSum by replacing iterator chains with direct loops. Fixes #570, fixes #571 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix unresolved review issues from #569 and reduce allocations in GreedyFrequencyStrategy. Bug fixes: - Add empty headers guard in GreedyFrequencyStrategy to prevent panic - Replace lo.Must(lo.Nth()) with direct index access + bounds check to prevent panic on short rows - Always distribute remainder width (fall back to column 0) in GreedyFrequencyStrategy - Fix ProportionalStrategy to distribute remaining width when totalDeficit == 0 Performance: - Replace hiter.Map/hiter.Concat/slices.Collect chain in transposedRows with direct slice construction - Replace lo.Sum with inline sumWidths to avoid evaluation on every slog.Debug call - Optimize adjustToSum: compute sorted unique thresholds once, inline clipping + sum - Remove unused clipToMax function Tests: - Add TestGreedyFrequencyStrategy_ShortRows, TestProportionalStrategy_ZeroDeficit, TestGreedyFrequencyStrategy_FullWidth Fixes #570, fixes #571
Document the new pluggable width strategy system variable added in #569. Add CLI_WIDTH_STRATEGY entry to docs/system_variables.md with all three algorithm options and usage notes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Extract the monolithic
calculateOptimalWidth()behind aWidthStrategyinterface, enabling runtime selection of column width algorithms via theCLI_WIDTH_STRATEGYsystem variable. This is a prerequisite for #567 (NoWrapCell).Three strategies are implemented:
container/heap)Key Changes
WidthStrategyenum withgo:generatedirectiveWidthStrategyinterface,ColumnHintstruct (withNoWrapreserved for IntroduceNoWrapCellto generalizeminColumnWidthhardcoding #567), andNewWidthStrategyfactorycalculateOptimalWidth()body intoGreedyFrequencyStrategy.CalculateWidths()ProportionalStrategy— allocates proportionally to each column's deficit from natural widthMarginalCostStrategy— greedy heap-based wrap-line minimization, O(availableWidth * log(numCols))calculateOptimalWidth()replaced by thinCalculateWidthWithStrategy()that computes overhead, creates hints, delegates to strategy. Also fixes header double-counting:verboseHeadersare now passed asheadersto strategies (counted once), instead of being prepended to rows and re-added by each strategy.WidthStrategy enums.WidthStrategytoFormatConfigcalculateWidths()WidthStrategytoDisplayVars, wire intoFormatConfig()WidthStrategyVar()helperCLI_WIDTH_STRATEGYsystem variableBug Fix: Header Double-Counting
The original
calculateOptimalWidth()prependedverboseHeaderstorows, then re-addedcolumnNamesto the data inside the algorithm viahiter.Concat(hiter.Once(StringsToRow(header...)), ...). This caused headers to be counted twice in width calculations (especially in non-verbose mode whereverboseHeaders == columnNames).Fixed by passing
verboseHeadersas theheadersparameter to strategies (which include them exactly once in their data), without prepending torows. In verbose mode,adjustByHeadernow correctly uses verbose header display widths for initial proportional allocation.Test Plan
make checkpasses (test + lint + fmt-check)width_test.gotests pass unchanged (regression safety)TestCompareStrategieslogs meaningful output for all 3 strategiesTestStrategyMinColumnWidthensures all strategies respectminColumnWidthTestSystemVariables_EnumTypes/WidthStrategytests SET with all values, case insensitivity, and invalid value rejectionFixes #566