posit-dev / great-tables

Make awesome display tables using Python.
https://posit-dev.github.io/great-tables/
MIT License
1.9k stars 72 forks source link

Improve `GT.save()` usability #499

Closed jrycw closed 3 days ago

jrycw commented 4 weeks ago

Related PR: #496.

This PR introduces several improvements to enhance the usability of GT.save():

Additionally, I noticed a potential speed boost (approximately 40-50% faster on my machine) when calling headless_browser=wdriver(options=wd_options) directly, bypassing the context manager. However, I'm unsure about the safety implications of this approach.

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 91.52542% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.38%. Comparing base (1ad3ec1) to head (23f12cb). Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
great_tables/_utils_selenium.py 90.90% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #499 +/- ## ========================================== + Coverage 87.86% 89.38% +1.52% ========================================== Files 42 45 +3 Lines 4852 5239 +387 ========================================== + Hits 4263 4683 +420 + Misses 589 556 -33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

jrycw commented 1 week ago

Hello team,

In my last commit, I experimented with isolating the web driver preparation logic into _utils_selenium.py. Please feel free to review and decide whether to keep or discard this change.

rich-iannone commented 1 week ago

This is looking good! I had a look at the output images on all 4 webdrivers (at 3 different scale values) to make sure outputs are similar to main. In case this is interesting I've included the images in this zip file:

image-tests.zip

I've found no reduction in quality with the changes here. Chrome is the only one that is problematic (cuts off content at the bottom) but that's also on main and an open issue anyway.

jrycw commented 4 days ago

This is looking good! I had a look at the output images on all 4 webdrivers (at 3 different scale values) to make sure outputs are similar to main. In case this is interesting I've included the images in this zip file:

image-tests.zip

I've found no reduction in quality with the changes here. Chrome is the only one that is problematic (cuts off content at the bottom) but that's also on main and an open issue anyway.

As a side note, this PR does not address the cutoff bug encountered when using Chrome, as mentioned in #480 .

machow commented 3 days ago

Shoot, I just noticed the change to passing a base64 encoded url directly to the headless browser. I think there could be some challenges with this approach (and url length limitations). In general, modern browsers seem to have large limits, but it's a tricky territory.

Opened an issue to track:

https://github.com/posit-dev/great-tables/issues/518