feat: quarto option to disable table processing, warn on render #611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
+ Coverage 90.71% 90.91% +0.20%
==========================================
Files 46 47 +1
Lines 5417 5440 +23
==========================================
+ Hits 4914 4946 +32
+ Misses 503 494 -9 ☔ View full report in Codecov by Sentry. |
|
@cscheid do you mind taking a look at these checks, and letting me know if they look reasonable for Quarto? We could always add a doc page to Great Tables that this warning links to, etc..?! |
|
I'll take a look right away, thanks. |
great_tables/_render_checks.py
Outdated
| return | ||
|
|
||
| # cols_widths set ---- | ||
| if any([col.column_width is not None for col in data._boxhead]): |
There was a problem hiding this comment.
The width configuration still works fine in Pandoc if it's given in percentages (which is why I wanted to do the fixup in the previous PR as I did). It's only when they're given in pixels that Pandoc doesn't like to parse them.
If I understand the code correctly here, the warning you're giving is maybe too generic, since it will warn in cases where things would still work fine.
There was a problem hiding this comment.
Ah, thanks! I'll add a piece that skips the warning if the user puts only percentages, and clarify when I warn that it's because non-percentages were used...
| col_widths = [col.column_width for col in data._boxhead] | ||
|
|
||
| is_any_set = any([width is not None for width in col_widths]) | ||
| is_all_pct = all([width is None or width.rstrip().endswith("%") for width in col_widths]) |
There was a problem hiding this comment.
We now skip the warning if all col widths are either unset or set to percentages
great_tables/_spanners.py
Outdated
| @@ -706,6 +706,8 @@ def cols_width(self: GTSelf, cases: dict[str, str] | None = None, **kwargs: str) | |||
| _assert_list_is_subset(mod_columns, set_list=column_names) | |||
|
|
|||
| for col, width in new_cases.items(): | |||
| if not isinstance(width, str): | |||
| raise ValueError(f"Width must be a string. Column {col} width received a {type(width)}") | |||
There was a problem hiding this comment.
@rich-iannone I noticed the annotations require width to be a string, but it's not enforced. WDYT of raising an error if a non-string is passed? If it's okay for users to pass a number, we could always coerce it to a string here instead!
There was a problem hiding this comment.
I definitely prefer that a string be the required input here. This is a good improvement, thanks!
There was a problem hiding this comment.
Upon discussion with @machow , we decided that a warning is more suitable here.
There was a problem hiding this comment.
My understanding is the R program allows integers, so we'll fire a deprecation warning and coerce to a string for a while, just in case this path gets used by folks....!
This PR would replace #603, and implements the following:
GT.tab_options(quarto_disable_processing=...). This was already in options and just needed to be added to the.tab_options()signature.Note that I also ran into an issue with imports being collapsed (from 1 individual piece imported on each line), and ended up having to disable my isort vs code extension.
But in the process I thrashed and upgraded ruff 😵. We can revert back if useful!