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

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

Closed
rich-iannone opened this issue Dec 2, 2024 · 1 comment

Comments

@rich-iannone
Copy link
Member

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)

@rich-iannone
Copy link
Member Author

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.

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

No branches or pull requests

1 participant