-
Notifications
You must be signed in to change notification settings - Fork 27
Implement "folded table" output, as an opt-in feature for all tabular outputs #1231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
36ff82a
Add an initial FoldedTablePrinter
sirosen 083b1e5
Refine output style in folded table printer
sirosen 53fd961
Add tests of folded table printer
sirosen 35d13a5
Add support for table folding via an env var
sirosen 5e89019
Invert the default for 'GLOBUS_CLI_FOLD_TABLES'
sirosen d2fae5b
Convert folded tables to box drawing chars
sirosen 92f416b
Add changelog for folded table output
sirosen e9c4e66
Switch folded table away from half-dash character
sirosen 1685919
Convert folded table row printing to use row types
sirosen 088bac0
Remove 'OutputStyle' from folded table printer
sirosen a62eb7d
Add 'content_rows' to simplify RowTable usage
sirosen 19806f3
Abstract out handling of folded table row styles
sirosen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| ### Enhancements | ||
|
|
||
| * When using table output on narrow terminals, the Globus CLI will now stack | ||
| table elements in a new "folded table" layout. This behavior is only used | ||
| when the output device is a TTY. To disable the new output altogether, users | ||
| can set `GLOBUS_CLI_FOLD_TABLES=0`. |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,333 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import collections | ||
| import enum | ||
| import functools | ||
| import shutil | ||
| import typing as t | ||
|
|
||
| import click | ||
|
|
||
| from ..context import out_is_terminal, term_is_interactive | ||
| from ..field import Field | ||
| from .base import Printer | ||
|
|
||
|
|
||
| class OutputStyle(enum.Flag): | ||
| none = enum.auto() | ||
| decorated = enum.auto() | ||
| double = enum.auto() | ||
| double_transition = enum.auto() | ||
| top = enum.auto() | ||
| bottom = enum.auto() | ||
|
|
||
|
|
||
| class FoldedTablePrinter(Printer[t.Iterable[t.Any]]): | ||
| """ | ||
| A printer to render an iterable of objects holding tabular data with cells folded | ||
| together and stacked in the format: | ||
|
|
||
| ╒════════════════╤════════════════╤════════════════╕ | ||
| │ <field.name 1> ╎ <field.name 3> ╎ <field.name 5> │ | ||
| ├─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┤ | ||
| │ <field.name 2> ╎ <field.name 4> ╎ │ | ||
| ╞════════════════╪════════════════╪════════════════╡ | ||
| │ <obj.value 1> ╎ <obj.value 3> ╎ <obj.value 5> │ | ||
| ├─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┤ | ||
| │ <obj.value 2> ╎ <obj.value 4> ╎ │ | ||
| ├────────────────┼────────────────┼────────────────┤ | ||
| │ <obj.value 1> ╎ <obj.value 3> ╎ <obj.value 5> │ | ||
| ├─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┼─ ─ ─ ─ ─ ─ ─ ─┤ | ||
| │ <obj.value 2> ╎ <obj.value 4> ╎ │ | ||
| └────────────────┴────────────────┴────────────────┘ | ||
|
|
||
| Rows are folded and stacked only if they won't fit in the output width. | ||
|
|
||
| :param fields: a list of Fields with load and render instructions; one per column. | ||
| """ | ||
|
|
||
| def __init__(self, fields: t.Iterable[Field], width: int | None = None) -> None: | ||
| self._fields = tuple(fields) | ||
| self._width = width or _get_terminal_content_width() | ||
| self._folding_enabled = _detect_folding_enabled() | ||
|
|
||
| def echo(self, data: t.Iterable[t.Any], stream: t.IO[str] | None = None) -> None: | ||
| """ | ||
| Print out a rendered table. | ||
|
|
||
| :param data: an iterable of data objects. | ||
| :param stream: an optional IO stream to write to. Defaults to stdout. | ||
| """ | ||
| echo = functools.partial(click.echo, file=stream) | ||
|
|
||
| table = self._fold_table(RowTable.from_data(self._fields, data)) | ||
| col_widths = table.calculate_column_widths() | ||
|
|
||
| # if folded, print a leading separator line | ||
| table_style = OutputStyle.decorated if table.folded else OutputStyle.none | ||
|
|
||
| if table.folded: | ||
| echo( | ||
| _separator_line( | ||
| col_widths, style=table_style | OutputStyle.double | OutputStyle.top | ||
| ) | ||
| ) | ||
| # print the header row and a separator (double if folded) | ||
| echo( | ||
| table.header_row.serialize( | ||
| col_widths, | ||
| style=table_style | ||
| | (OutputStyle.double if table.folded else OutputStyle.none), | ||
| ) | ||
| ) | ||
| echo( | ||
| _separator_line( | ||
| col_widths, | ||
| style=( | ||
| table_style | ||
| | ( | ||
| OutputStyle.double_transition | ||
| if table.folded | ||
| else OutputStyle.none | ||
| ) | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| # if the table is empty, print nothing, but normally there is more than one row | ||
| if len(table.rows) > 1: | ||
|
sirosen marked this conversation as resolved.
Outdated
|
||
| for row in table.rows[1:-1]: | ||
| echo(row.serialize(col_widths, style=table_style)) | ||
| if table.folded: | ||
| echo(_separator_line(col_widths, style=table_style)) | ||
| echo(table.rows[-1].serialize(col_widths, style=table_style)) | ||
| if table.folded: | ||
| echo(_separator_line(col_widths, style=table_style | OutputStyle.bottom)) | ||
|
|
||
| def _fold_table(self, table: RowTable) -> RowTable: | ||
| if not self._folding_enabled: | ||
| return table | ||
|
|
||
| # if the table is initially narrow enough to fit, do not fold | ||
| if table.fits_in_width(self._width): | ||
| return table | ||
|
|
||
| # try folding the table in half, and see if that fits | ||
| folded_table = table.fold_rows(2) | ||
| if folded_table.fits_in_width(self._width): | ||
| return folded_table | ||
| # if it's still too wide, fold in thirds and check that | ||
| else: | ||
| folded_table = table.fold_rows(3) | ||
| if folded_table.fits_in_width(self._width): | ||
| return folded_table | ||
| # if folded by thirds does not fit, fold all the way to a single column | ||
| else: | ||
| return table.fold_rows(table.num_columns) | ||
|
|
||
|
|
||
| @functools.cache | ||
| def _separator_line( | ||
| col_widths: tuple[int, ...], style: OutputStyle = OutputStyle.none | ||
| ) -> str: | ||
| fill = "=" if style & (OutputStyle.double | OutputStyle.double_transition) else "-" | ||
|
|
||
| if style & OutputStyle.decorated: | ||
| # remap fill to a box drawing char | ||
| fill = {"=": "═", "-": "─"}[fill] | ||
|
|
||
| before_decorator = "├" | ||
| after_decorator = "┤" | ||
| middle_decorator = "┼" | ||
| if style & OutputStyle.top: | ||
| before_decorator = "╒" | ||
| after_decorator = "╕" | ||
| middle_decorator = "╤" | ||
| elif style & OutputStyle.bottom: | ||
| before_decorator = "└" | ||
| after_decorator = "┘" | ||
| middle_decorator = "┴" | ||
| elif style & OutputStyle.double_transition: | ||
| before_decorator = "╞" | ||
| after_decorator = "╡" | ||
| middle_decorator = "╪" | ||
|
sirosen marked this conversation as resolved.
Outdated
|
||
|
|
||
| leader = f"{before_decorator}{fill}" | ||
| trailer = f"{fill}{after_decorator}" | ||
| else: | ||
| leader = "" | ||
| trailer = "" | ||
| middle_decorator = "+" | ||
|
|
||
| line_parts = [leader] | ||
| for col in col_widths[:-1]: | ||
| line_parts.append(col * fill) | ||
| line_parts.append(f"{fill}{middle_decorator}{fill}") | ||
| line_parts.append(col_widths[-1] * fill) | ||
| line_parts.append(trailer) | ||
| return "".join(line_parts) | ||
|
|
||
|
|
||
| class RowTable: | ||
| """ | ||
| A data structure to hold tabular data which has not yet been laid out. | ||
|
|
||
| :param rows: a list of rows with table's contents, including the header row | ||
| :param folded: whether or not the table has been folded at all | ||
| :raises ValueError: if any rows have different numbers of columns. | ||
| """ | ||
|
|
||
| def __init__(self, rows: tuple[Row, ...], folded: bool = False) -> None: | ||
| self.rows = rows | ||
| self.folded = folded | ||
|
|
||
| self.num_columns = rows[0].num_cols | ||
| self.num_rows = len(rows) | ||
|
|
||
| @property | ||
| def header_row(self) -> Row: | ||
| return self.rows[0] | ||
|
sirosen marked this conversation as resolved.
|
||
|
|
||
| def fits_in_width(self, width: int) -> bool: | ||
| return all(x.min_rendered_width <= width for x in self.rows) | ||
|
|
||
| def fold_rows(self, n: int) -> RowTable: | ||
| """Produce a new table with folded rows.""" | ||
| return RowTable(tuple(cell.fold(n) for cell in self.rows), folded=True) | ||
|
|
||
| def calculate_column_widths(self) -> tuple[int, ...]: | ||
| return tuple( | ||
| max(0, *(self.rows[row].column_widths[col] for row in range(self.num_rows))) | ||
| for col in range(self.rows[0].num_cols) | ||
| ) | ||
|
|
||
| @classmethod | ||
| def from_data(cls, fields: tuple[Field, ...], data: t.Iterable[t.Any]) -> RowTable: | ||
| """ | ||
| Create a RowTable from a list of fields and iterable of data objects. | ||
|
|
||
| The data objects are serialized and discarded upon creation. | ||
| """ | ||
| rows = [] | ||
| # insert the header row | ||
| rows.append(Row((tuple(f.name for f in fields),))) | ||
| for data_obj in data: | ||
| rows.append(Row.from_source_data(fields, data_obj)) | ||
|
|
||
| return cls(tuple(rows)) | ||
|
|
||
|
|
||
| class Row: | ||
| """A semantic row in the table of output, with a gridded internal layout.""" | ||
|
|
||
| def __init__(self, grid: tuple[tuple[str, ...], ...]) -> None: | ||
| self.grid: tuple[tuple[str, ...], ...] = grid | ||
|
|
||
| def __len__(self) -> int: | ||
| return sum(len(subrow) for subrow in self.grid) | ||
|
|
||
| def __getitem__(self, coords: tuple[int, int]) -> str: | ||
| subrow, col = coords | ||
| return self.grid[subrow][col] | ||
|
|
||
| @classmethod | ||
| def from_source_data(cls, fields: tuple[Field, ...], source: t.Any) -> Row: | ||
| return cls((tuple(field.serialize(source) for field in fields),)) | ||
|
|
||
| def fold(self, n: int) -> Row: | ||
| """Fold the internal grid by N, stacking elements. Produces a new Row.""" | ||
| if self.is_folded: | ||
| raise ValueError( | ||
| "Rows can only be folded once. Use the original row to refold." | ||
| ) | ||
| return Row(tuple(self._split_level(self.grid[0], n))) | ||
|
|
||
| def _split_level( | ||
| self, level: tuple[str, ...], modulus: int | ||
| ) -> t.Iterator[tuple[str, ...]]: | ||
| bins = collections.defaultdict(list) | ||
| for i, x in enumerate(level): | ||
| bins[i % modulus].append(x) | ||
|
|
||
| for i in range(modulus): | ||
| yield tuple(bins[i]) | ||
|
|
||
| @functools.cached_property | ||
| def is_folded(self) -> bool: | ||
| return len(self.grid) > 1 | ||
|
|
||
| @functools.cached_property | ||
| def min_rendered_width(self) -> int: | ||
| decoration_length = 0 | ||
| if self.is_folded: | ||
| decoration_length = 4 | ||
| return sum(self.column_widths) + (3 * (self.num_cols - 1)) + decoration_length | ||
|
|
||
| @functools.cached_property | ||
| def num_cols(self) -> int: | ||
| return max(0, *(len(subrow) for subrow in self.grid)) | ||
|
|
||
| @functools.cached_property | ||
| def column_widths(self) -> tuple[int, ...]: | ||
| """The width of all columns in the row (as measured in this row).""" | ||
| return tuple(self._calculate_col_width(i) for i in range(self.num_cols)) | ||
|
|
||
| def _calculate_col_width(self, idx: int) -> int: | ||
| return max( | ||
| 0, *(len(subrow[idx]) if idx < len(subrow) else 0 for subrow in self.grid) | ||
| ) | ||
|
|
||
| def serialize( | ||
| self, use_col_widths: tuple[int, ...], style: OutputStyle = OutputStyle.none | ||
| ) -> str: | ||
| lines: list[str] = [] | ||
|
|
||
| separator_line: list[str] = [] | ||
| if len(self.grid) > 1: | ||
| for width in use_col_widths: | ||
| separator_line.append(_make_row_separator(width)) | ||
|
|
||
| for i, subrow in enumerate(self.grid): | ||
| new_line: list[str] = [] | ||
| for idx, width in enumerate(use_col_widths): | ||
| new_line.append((subrow[idx] if idx < len(subrow) else "").ljust(width)) | ||
|
|
||
| if style & OutputStyle.decorated: | ||
| separator = "╎" | ||
| leader = "│ " | ||
| trailer = " │" | ||
|
|
||
| if i > 0 and separator_line: | ||
| lines.append("├─" + "─┼─".join(separator_line) + "─┤") | ||
| else: | ||
| leader = "" | ||
| trailer = "" | ||
| separator = "|" | ||
| lines.append(leader + f" {separator} ".join(new_line) + trailer) | ||
| return "\n".join(lines) | ||
|
|
||
|
|
||
| _ROW_SEPARATOR_CHAR = "─" | ||
|
|
||
|
|
||
| @functools.cache | ||
| def _make_row_separator(width: int) -> str: | ||
| # repeat with whitespace | ||
| sep = (" " + _ROW_SEPARATOR_CHAR) * width | ||
| sep = sep[:width] # trim to length | ||
| if sep[-1] == _ROW_SEPARATOR_CHAR: # ensure it ends in whitespace | ||
| sep = sep[:-1] + " " | ||
| return sep | ||
|
|
||
|
|
||
| def _get_terminal_content_width() -> int: | ||
| """Get a content width for text output based on the terminal size. | ||
|
|
||
| Uses the 90% of terminal width, if it can be detected. | ||
| """ | ||
| cols = shutil.get_terminal_size(fallback=(80, 20)).columns | ||
| return cols if cols < 88 else int(0.9 * cols) | ||
|
|
||
|
|
||
| def _detect_folding_enabled() -> bool: | ||
| return out_is_terminal() and term_is_interactive() | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being represented as a boolean flag makes it pretty difficult to understand.
From a lot of back and forth, I think I get the idea is that:
noneanddecorateddisambiguate broadly "how am I styling this"topandbottomdelineate (for specifically separator lines) whether to modify the cornersdoubleanddouble_transitiondelineate stylization characters within a separator line.Assuming I got that right, it took a lot of imperative piecing together to figure out.
Maybe some comments would help a reader grok this quicker but I think it's just not a great way to model this. The approach merges different abstraction levels (e.g., "decorated" and "bottom") into the same "bag" of style-elements, requiring the reader to look at all of the usage sites to understand anything about what they are.
My recommendation would be to model the "style of output" (e.g.,
noneordecorated) distinctly from the "category of thing being output".Something like the following would convey the same information more explicitly in method signatures with informative types of stylization than the permutation bag of styles offers.
Example Note:
I used literal types just because that helped me include the data type in the signature; for a real implementation those could be modeled as enums or dataclasses or whatever. The important part I'm trying to highlight is that different "types" of stylization are identified concretely in the signature instead of mushed together into style flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth noting that
stylein this pared downnonevsdecoratedlooks like it's just a renaming ofis_folded. Keeping it as a bool instead of a renamed "style type" may help remove an abstraction that actually helps disambiguate at usage site why we might be renderingnone-style vsdecorated-style.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to tinker on this now. My first step is going to be to try removing
decoratedas a style -- using theis_foldedbool more -- and see how that looks. After that, I think I will want to separate "style" from "line type" (which looks to me like the right split).I'm sticking with enums, but as you say that's a detail. 🤞 it goes smoothly, but I agree with the overall feedback. Too many ideas got mushed into one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a version of this, but, unlike the other threads, I'm leaving it unresolved until you have a look. There's a new enum for "separator row type", and I feel good about the changes, but it's not how I imagined this going when I first was digging in. If you find the results odd, let's iterate one more time at least.