fix: apply column max widths when rendering tables (#88) - #542
Open
MohammadYusif wants to merge 1 commit into
Open
fix: apply column max widths when rendering tables (#88)#542MohammadYusif wants to merge 1 commit into
MohammadYusif wants to merge 1 commit into
Conversation
`Table.set_column_max_width()` stored the width in `_column_widths` instead of `_column_max_widths`. Since `_column_max_widths` was read in `_build_table_rows()` and `_get_cell_width()` but never written, the max-width and cell-wrapping machinery was unreachable: a wide column was never capped, and because `_get_cell_width()` folds `_column_widths` in with `max()`, the value acted as a *minimum* width and padded narrow columns out instead. Routing the setter to the right attribute exposes a second defect in the now-reachable wrapping branch: it asserted the table's io was an `Output`, but `Table` accepts `IO | Output` and `Command.table()` passes an `IO`, so wrapping raised `AssertionError` for the common case. The formatter is now resolved from either, mirroring `IO.remove_format()`. This restores the documented way to fit a table that is wider than the terminal.
dosubot
Bot
added
the
size:XS
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
1 participant
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.
Summary
Table.set_column_max_width()wrote to_column_widthsinstead of_column_max_widths. Nothing else ever writes_column_max_widths, so the max-width and cell-wrapping code in_build_table_rows()was unreachable and setting a max width did nothing._get_cell_width()folds_column_widthsin withmax(), so the value actually acted as a minimum width —set_column_max_width(0, 20)padded a 2-character column out to 20, the inverse of the method name.self._iois anOutput, butTableis typedio: IO | OutputandCommand.table()passes anIO, which has noformatterproperty — so wrapping raisedAssertionErrorfor the common entry point. That assert had only ever satisfied mypy; it had never actually run.Changes
src/cleo/ui/table.py:set_column_max_width()now writes_column_max_widths; the wrapping branch resolves the formatter from either anIOor anOutput, mirroring howIO.remove_format()delegates.tests/ui/test_table.py: addedtest_column_max_width_wraps_long_cells(a long cell is capped and wrapped) andtest_column_max_width_does_not_widen_short_cells(a max width must not pad a narrow column). Both fail onmain.news/88.bugfix.md: news fragment.I left
_cleanup()alone on purpose — it resets_column_widths, but Symfony'sTable::cleanup(), which this ports, resets neither. Happy to change that if you'd prefer both setters behave the same way.Scope
Refs #88. To be precise about what this does and does not do: the literal
ValueErrorfrom that 2020 report is already gone, since it came from stdlibtextwrap, which no longer appears anywhere in cleo. What is still broken is that issue's own option C — cleo has the machinery to shrink a too-wide table, it just never runs. This makes it run. It does not add automatic fitting to terminal width (option A); a caller still has to set a max width explicitly. Leaving the issue open for that reason rather than auto-closing it.