posit-dev/great-tables

[pyOpenSci review] use `hasattr(x, "to_html")` instead of checking for a `UnitStr` instance

rich-iannone opened this issue · 1 comments

From the pyOpenSci comment:

suggestion (_text.py): I wonder if we could use hasattr(x, "to_html") instead of checking for a UnitStr instance. That way we would 1. get rid of the circular import and 2. enable other types. I totally understand if you want to keep full control of what is rendered, though.

pyOpenSci/software-submission#202 (comment)

At the time of review this was:

https://github.com/posit-dev/great-tables/blob/32998e8fd26296da76be512e56b76ae8a59d4fa8/great_tables/_text.py#L54C1-L71C52

def _process_text(x: str | Text | None) -> str:
    from great_tables._helpers import UnitStr

    if x is None:
        return ""

    if isinstance(x, Md):
        return _md_html(x.text)
    elif isinstance(x, Html):
        return x.text
    elif isinstance(x, str):
        return _html_escape(x)
    elif isinstance(x, Text):
        return x.text
    elif isinstance(x, UnitStr):
        return x.to_html()
    else:
        raise TypeError(f"Invalid type: {type(x)}")

Now, we've moved to using a BaseText class and this greatly simplifies the HTML rendering:

def _process_text(x: str | BaseText | None, context: str = "html") -> str:

def _process_text(x: str | BaseText | None, context: str = "html") -> str:
    if x is None:
        return ""

    escape_fn = _html_escape if context == "html" else _latex_escape

    if isinstance(x, str):
        return escape_fn(x)

    elif isinstance(x, BaseText):
        return x.to_html() if context == "html" else x.to_latex()

    raise TypeError(f"Invalid type: {type(x)}")

This work removes the circular import and enables other types.