Fix: cols_width() should exclude hidden columns

[rich-iannone]

This fixes #491, and the problem is that hidden columns are included in the <colgroup> tag when only visible columns should be included.

With the fix, the following table renders correctly (adapted from the issue's table):

from great_tables import GT, exibble

exibble_mini = exibble[["num", "char", "date", "datetime", "row"]].head(1)

exibble_mini["long_text"] = (
    "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."
)


tbl = (
    GT(exibble_mini)
    .cols_hide(columns="datetime")
    .cols_width(
        cases={
            "num": "50px",
            "char": "50px",
            "date": "400px",
            "row": "50px",
            "long_text": "400px",
            "datetime": "20px"}
    )
    .tab_options(
        table_body_vlines_style="solid",
        table_body_vlines_width="3px",
        table_body_vlines_color="red"
    )
)

tbl

The colgroup here is now:

<colgroup>
  <col style="width:50px;"/>
  <col style="width:50px;"/>
  <col style="width:400px;"/>
  <col style="width:50px;"/>
  <col style="width:400px;"/>
</colgroup>

which matches the number of columns and has the correct widths for each of the columns. Including or excluding a column width definition for a hidden column in the GT code makes no difference to the final result.

There was also another bug addressed with column widths and it involves not writing a column width in <colgroup> for a stub column. This PR fixes that by retrieving the column width (if any) for a stub column and placing it at the beginning of the series in <colgroup>. Here's a revised version of the above code that uses the "date" column as the row label column in the stub:

...

tbl = (
    GT(exibble_mini, rowname_col="date")
...

Here's the resulting <colgroup>:

<colgroup>
  <col style="width:400px;"/>
  <col style="width:50px;"/>
  <col style="width:50px;"/>
  <col style="width:50px;"/>
  <col style="width:400px;"/>
</colgroup>

Fixes: #491

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (336e688) to head (f61f662).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   88.91%   89.11%   +0.20%     
==========================================
  Files          44       44              
  Lines        5212     5220       +8     
==========================================
+ Hits         4634     4652      +18     
+ Misses        578      568      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[machow]

Could we add a method like Boxhead.canonical_columns() or .final_columns(), that returns a list[ColInfo] for all final visible columns (including stub, and group column if correct option is set), in the order they will be rendered in?

This way, Boxhead becomes the source of truth on the final columns. I realize there's some ambiguity between Stub and Boxhead columns. But it seems okay for the final columns across these two things to live in one place, and the Boxhead seems like it might be the best fit?

In this case, the code could just call something like...

columns = data._boxhead.canonical_columns(options)

# widths for all columns that will be represented as columns
# e.g. groupings are rows by default, so wouldn't be included,
# but if the option to make them a column is set, then they would
widths = [col.width for col in columns]

[rich-iannone]

Now done, thanks for your help on this!

[machow]

What if rather than...

• get visible columns
• get stub
• if stub exists, remove from visible columns and create a new list of columns with stub at the front
• get group column
• if group column exists do something similar (shifting around columns)

We constructed the final column list directly, so something like:

• get stub column if it exists and append to list
• get row group column if it exists (and option dictates its a column) and append to list
• extend list with remaining columns (those marked ColInfoTypeEnum.default?)

This way the code reads in the same order as the final columns returned

[machow]

Can we return a list of columns for the result of final_columns? It seems likely this boxhead with columns dropped (rather than hidden) violates some assumption of our data model.

[rich-iannone]

Now done. Thanks!

[machow]

What if we just return each x, rather than x.var? That way people can use the ColInfo directly to get any pieces they needed, rather than fetching ColInfo later off the boxhead?

[rich-iannone]

Good idea. Now implemented.

[machow]

This looks great, thanks!