jazzband / prettytable

Display tabular data in a visually appealing ASCII table format
https://pypi.org/project/PrettyTable/
Other
1.32k stars 149 forks source link

max_table_width does not work as expected #288

Closed stuertz closed 3 months ago

stuertz commented 4 months ago

What did you do?

Set max_table_width on a table to os.get_terminal_size().columns and played around with terminal size.

What did you expect to happen?

The table should not exceed max_table_width (no line breaks in terminal)

max_width_should be max.

What actually happened?

Sometimes tables are wider than terminal size and i see line breaks

What versions are you using?

hugovk commented 4 months ago

It's a bit hard to know what's happening without example code or screenshots, but PrettyTable isn't intended as a text-based user interface (TUI) but for outputting static tables.

If you've set it to os.get_terminal_size().columns, and then later that value changes, we've already read in the old width. https://textual.textualize.io/ might be a better fit for a TUI.

stuertz commented 4 months ago

Yep examples are missing. I'll try to add some with non confidential, static data.

I'll agree, that changing terminal window size during or after the call would lead to unexpected results. But I didn't change it during the call.

Mostly i saw the table is one or two bytes wider than requested. But yes, I will provide an example ASAP

stuertz commented 4 months ago

E.g. this example puts the table width to 217, instead 214 was requested:

import prettytable
import os

WIDTH = 214

table = prettytable.PrettyTable(
    ("Effort", "Customer", "Project", "Class", "Name", "Comment"),
    max_table_width=WIDTH,
    align="l",
)
table.align["Effort"] = "r"
table.add_row(
    [
        "1.5",
        "123456",
        "Customer A",
        "Project A",
        "Sales and Marketingsupport",
        "Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua",
    ]
)
out = table.get_string()
print(f"Terminal; {os.get_terminal_size().columns}")
print(f"Defined: {WIDTH}")

print(out)
for line in out.split("\n"):
    print(len(line))
    assert len(line) <= WIDTH

Output:

Terminal; 214
Defined: 214
+-------+---------+-----------+----------+--------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------+
| Effor | Custome | Project   | Class    | Name                     | Comment                                                                                                                                           |
+-------+---------+-----------+----------+--------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------+
|   1.5 | 123456  | Customer  | Project  | Sales and                | Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam |
|       |         | A         | A        | Marketingsupport         | voluptua                                                                                                                                          |
+-------+---------+-----------+----------+--------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------+
217
Traceback (most recent call last):
  File "/Users/js/repos/timetraquer/src/timetraquer/prettytest.py", line 30, in <module>
    assert len(line) <= WIDTH
AssertionError
stuertz commented 4 months ago

This would be the appropriate pytest:

    def test_max_table_width_wide(self) -> None:
        table = PrettyTable()
        table.max_table_width = 52
        table.add_row(
            [
                0,
                0,
                0,
                0,
                0,
                "Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua",
            ]
        )

        assert (
            table.get_string().strip()
            == """
+---+---+---+---+---+------------------------------+
| F | F | F | F | F |           Field 6            |
+---+---+---+---+---+------------------------------+
| 0 | 0 | 0 | 0 | 0 | Lorem ipsum dolor sit amet,  |
|   |   |   |   |   | consetetur sadipscing elitr, |
|   |   |   |   |   |    sed diam nonumy eirmod    |
|   |   |   |   |   | tempor invidunt ut labore et |
|   |   |   |   |   | dolore magna aliquyam erat,  |
|   |   |   |   |   |      sed diam voluptua       |
+---+---+---+---+---+------------------------------+""".strip()
        )
hugovk commented 4 months ago

Thanks, it looks like we're not accounting for the dividing characters when computing the width of the table.

Something like this accounts for them when deciding whether we need to scale down:

     def _compute_table_width(self, options):
         table_width = 2 if options["vrules"] in (FRAME, ALL) else 0
-        per_col_padding = sum(self._get_padding_widths(options))
+        per_col_padding = sum(self._get_padding_widths(options)) + len(self.vertical_char)
         for index, fieldname in enumerate(self.field_names):
             if not options["fields"] or (
                 options["fields"] and fieldname in options["fields"]
             ):
                 table_width += self._widths[index] + per_col_padding
-        return table_width
+        return table_width + len(self.vertical_char)

And it's used here:

https://github.com/jazzband/prettytable/blob/6498a578ba81ccb45d741905d6ef675b1b53a709/src/prettytable/prettytable.py#L1593-L1600

And we find a scale ratio to shrink by.

But then we only apply it to the widths of the table contents, without taking account the padding or dividers.

stuertz commented 4 months ago

Same applies for the _compute_widths() method. I added tests and tried to fix it over the weekend, but i still have a width diff of two chars.

Feel free to have a look at my current changes: https://github.com/jazzband/prettytable/pull/289

additionally I have this change which tries to fix the main issue, but still has a diff of two chars and looks to complicated to me:

--- a/src/prettytable/prettytable.py
+++ b/src/prettytable/prettytable.py
@@ -1605,7 +1605,17 @@ class PrettyTable:
             table_width = self._compute_table_width(options)
             if table_width > self._max_table_width:
                 # Shrink widths in proportion
-                scale = 1.0 * self._max_table_width / table_width
+                base_table_width = 2 if options["vrules"] in (FRAME, ALL) else 0
+                # FIXME: Maybe broken with FRAME
+                scale = (
+                    self._max_table_width
+                    - base_table_width
+                    - per_col_padding * len(self.field_names)
+                ) / (
+                    table_width
+                    - base_table_width
+                    - per_col_padding * len(self.field_names)
+                )
                 widths = [int(w * scale) for w in widths]
                 self._widths = widths

Any hint is appreciated!

stuertz commented 4 months ago

Hi, MR has now a potential fix, thx for review!