posit-dev / great-tables

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

Update `FmtImage.to_html()` #444

Closed jrycw closed 2 months ago

jrycw commented 2 months ago

Hello team,

I'd like to format the column by providing an HTTP/HTTPS URL to the path parameter in GT.fmt_image() like so:

import polars as pl
from great_tables import GT

df = pl.DataFrame({"logo": ["GT_logo.svg"]})
GT(df).fmt_image(
    "logo",
    path="https://raw.githubusercontent.com/posit-dev/great-tables/main/docs/assets/",
)

I was expecting this outcome: image

However, I'm encountering a TypeError: sub() missing 1 required positional argument: 'string'.

It appears that the error originates from this line: https://github.com/posit-dev/great-tables/blob/016c67f7cf75272ac4bff0b678383a8f8993c985/great_tables/_formats.py#L3529

If I understand correctly, this line is intended to do two things: (1) remove trailing spaces, if any, and (2) remove the trailing /, if present. To fix this, the line could be updated as follows:

norm_path = re.sub(r"/\s*$", "", self.path)

This would:

That said, I believe using self.path.rstrip().removesuffix("/") as proposed in this PR is more straightforward and easier to understand.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.72%. Comparing base (016c67f) to head (8fa2fd4). Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #444 +/- ## ========================================== + Coverage 86.68% 86.72% +0.03% ========================================== Files 42 42 Lines 4685 4684 -1 ========================================== + Hits 4061 4062 +1 + Misses 624 622 -2 ```

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

machow commented 2 months ago

@rich-iannone do you mind taking a look? Especially on double checkingnorm_path's job there? Seems like a nice addition!

(may be useful for us to leave a note here on what would happen in the gt r package if someone used an http path)