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

fix: fmt cannot accept multiple fns #327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jrycw
Copy link
Collaborator

@jrycw jrycw commented May 5, 2024

Currently , the following code will raise AttributeError: 'list' object has no attribute 'default'.

from great_tables import GT, exibble

(
    GT(exibble[["fctr"]])
    .fmt([lambda x: x], columns="fctr")
)

The main issue appears to be that fmt did not handle fns properly when it is a list. As a result, the callables are not wrapped in FormatFns. Additionally, Body.render_formats needs to be refactored to accommodate this change by retrieving the result of _set_cell for each call. Given the critical role of fmt within this project, while the proposed PR may not be flawless, it serves as a reference for our refactoring efforts. With some snapshot tests failing, I would greatly appreciate the team's assistance in reviewing the PR to ensure the failed tests are acceptable.

@jrycw jrycw changed the title Fix issue where fmt cannot accept multiple fns fix: fmt cannot accept multiple fns May 5, 2024
Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jrycw, thanks for taking a look at this aspect of formatting. I left a comment explaining a bit more, but I think a list of formatting functions isn't expected (but an unfortunate bit of documentation copied from R made it sound like the case ☹️ )

It seems like some of the failing snapshots have changed because their formatting isn't running anymore, but I'm not sure why (based on the new code). For example,

def test_html_string_generated(gt_tbl: GT, snapshot: str):
>       assert snapshot == gt_tbl.as_raw_html()
E       assert [- snapshot] == [+ received]
E           '''
E             ...
E               <td class="gt_row gt_left">apricot</td>
E         -     <td class="gt_row gt_right">$49.95</td>
E         +     <td class="gt_row gt_right">49.95</td>
E             </tr>
E              ...
E               <td class="gt_row gt_left">banana</td>
E         -     <td class="gt_row gt_right">$17.95</td>
E         +     <td class="gt_row gt_right">17.95</td>
E             </tr>

In this output, it seems like the dollar signs are expected, since it runs the fmt_currency() method.

@@ -197,11 +197,13 @@ def _(data, row: int, column: str, value: Any) -> None:
# if this is violated, get_loc will return a mask
col_indx = data.columns.get_loc(column)
data.iloc[row, col_indx] = value
return data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since _set_cell() mutates data, it seems useful to continue not returning data (sometimes called Command query separation).

formatter = FormatInfo(fns, col_res, row_pos)
# If a single function is supplied to `fns` then
# repackage that into a list as the `default` function
formatter: list[FormatInfo] = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unfortunately GT.fmt never expects a list of functions as inputs, but uses the maybe trickily named FormatFns to mean, a dataclass that holds functions for different renderers (e.g. html, in the future latex, etc..).

It's a good point though that GT.fmt is an important entry point, so it's worth thinking carefully about its behavior. I think if it comes down to it, people could always pass lambda x: formatter2(formatter1(x))? Or use some implementation of a compose() function?

These docs:

    Parameters
    ----------
    fns
        Either a single formatting function or a named list of functions.

Unfortunately are copied over from R, and so named list is referring to something more like a dictionary (which we're currently using the FormatFns dataclass for)

Copy link
Collaborator Author

@jrycw jrycw May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the possibility of implementing a compose() function. In my imagination, the code might look something like this:

from functools import partial
from typing import Any, Callable
from great_tables import GT, exibble


def compose(funcs: list[Callable[Any, Any]]) -> Callable:
    def _compose(x: Any, funcs: list[Callable[Any, Any]]) -> Any:
        for f in funcs:
            x = f(x)
        return x
    return partial(_compose, funcs=funcs)


(
    GT(exibble[["fctr"]]).fmt(
        compose([lambda x: f"_{x}_", lambda x: f"*{x}*", lambda x: f"<{x}>"]),
        columns="fctr",
    )
)

image

@jrycw
Copy link
Collaborator Author

jrycw commented May 8, 2024

@machow , thank you so much for explaining the details to me. I believe making a small modification to the docs will make it easier for people who haven't used {gt} before (like me). Since I'll be out of town for a few days, would you mind taking over the PR to either close it or modify the docs?

@machow
Copy link
Collaborator

machow commented May 11, 2024

I believe making a small modification to the docs will make it easier for people who haven't used {gt} before (like me)

That makes a ton of sense --- I can make sure to change the docstring Monday. I'll look back through to see if there are pieces to keep from the PR (since it seemed like there were some other improvements in here :)

@jrycw
Copy link
Collaborator Author

jrycw commented May 11, 2024

@machow, thanks a lot.

@machow machow self-assigned this Aug 26, 2024
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.

2 participants