Skip to content
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

feat: handle cols_width differently in Quarto environment #603

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cscheid
Copy link
Collaborator

@cscheid cscheid commented Feb 12, 2025

Summary

This PR adds special handling to cols_width when running in Quarto, so that Pandoc knows how to parse column width information.

I haven't added a pytest test yet because I'm unsure of how you prefer to test Quarto-specific functionality. Locally, I've tested by mocking a Quarto environment, lying about QUARTO_BIN_PATH. I've also done an integration test and can confirm the feature runs. But I'd like to add explicit tests.

Related GitHub Issues and PRs

Checklist

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 25.92593% with 20 lines in your changes missing coverage. Please review.

Project coverage is 90.39%. Comparing base (3d6ad09) to head (60aa5a4).

Files with missing lines Patch % Lines
great_tables/_spanners.py 25.92% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
- Coverage   90.71%   90.39%   -0.33%     
==========================================
  Files          46       46              
  Lines        5417     5443      +26     
==========================================
+ Hits         4914     4920       +6     
- Misses        503      523      +20     

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

@rich-iannone
Copy link
Member

Thanks Carlos! Will review soon :)

@machow
Copy link
Collaborator

machow commented Feb 12, 2025

Thanks for taking the time to look into this and submit a PR! I noticed it...

  • can only set table width when all columns widths are set
  • disables quarto table processing otherwise

This seems a bit tricky to remember (e.g. "if I'm using quarto...", "if all widths are set...", "other option behavior set otherwise..."). What if we fired a warning instead?

  • when quarto is used with cols_width, and quarto table processing not disabled, then
  • display a warning (e.g. "you either need to manually set table width or disable table processing")

That way there's less risk of us blasting options users set, and they hopefully can to resolve the issue even when not all col widths are set. WDYT.

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 12, 2025

Thanks for the response!

Thanks for taking the time to look into this and submit a PR! I noticed it...

  • can only set table width when all columns widths are set
  • disables quarto table processing otherwise

I confirm that's the behavior I intended to implement. The reason is that we can only do the conversion to percentages if we know what the total width is intended to be. I agree that this doesn't cover the range of possible behaviors in HTML, but it covers the range of behavior that Pandoc understands.

This seems a bit tricky to remember (e.g. "if I'm using quarto...", "if all widths are set...", "other option behavior set otherwise..."). What if we fired a warning instead?

  • when quarto is used with cols_width, and quarto table processing not disabled, then
  • display a warning (e.g. "you either need to manually set table width or disable table processing")

That way there's less risk of us blasting options users set, and they hopefully can to resolve the issue even when not all col widths are set. WDYT.

I like it, but there are some tricky state transitions to think through. Disabling or enabling Quarto processing can be done after the column widths are set. So, for example, a user can, in principle, in the following order:

  1. disable quarto processing
  2. set cols_width in a way incompatible with Pandoc
  3. re-enable quarto processing

This would cause no warnings and yet would render a table that Pandoc will eventually incorrectly process.

I'll give warn() a shot.

@cscheid
Copy link
Collaborator Author

cscheid commented Feb 12, 2025

How do you want me to handle testing for this?

@machow
Copy link
Collaborator

machow commented Feb 12, 2025

This would cause no warnings and yet would render a table that Pandoc will eventually incorrectly process.

Users could also accidentally disable the things this PR sets in .tab_options() (and have the things they set there overridden by this behavior). I think getting people to the right result is super important. But worry a bit the GT state x rendering system x option setting interaction creates a lot of potential for surprising behaviors across the program.

I suspect our best bet is not to try and set quarto based workarounds, but to warn users right around when the bad behaviors are produced. For example, we could run a set of quarto specific warnings at render time.

What do you think of us not automatically setting any options, but running quarto-specific warning checks at render (or just before, whenever lets us detect more surprising GT x quarto behaviors)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants