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

Add the fmt_units() method #240

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Add the fmt_units() method #240

wants to merge 48 commits into from

Conversation

rich-iannone
Copy link
Member

@rich-iannone rich-iannone commented Mar 12, 2024

This PR adds the fmt_units() method. This performs a conversion of units in the units notation syntax (e.g., "x10^9 / L" or "x10^9 L^-1", etc.) to HTML. The method, like others of the fmt_*(), only concerns itself with text transformations in the table body. Other methods will also gain the ability to convert text in units notation to nicely formatted HTML in later PRs.

Here's an example that uses fmt_units() with the illness dataset. It so happens that the units column has strings in units notation, so, we just need to point this method to that column:

from great_tables import GT, style, loc
from great_tables.data import illness

(
    GT(illness, rowname_col="test")
    .fmt_units(columns="units")
    .fmt_number(columns=lambda x: x.startswith("day"), decimals=2, drop_trailing_zeros=True)
    .tab_header(title="Laboratory Findings for the YF Patient")
    .tab_spanner(label="Day", columns=lambda x: x.startswith("day"))
    .tab_spanner(label="Normal Range", columns=lambda x: x.startswith("norm"))
    .cols_label(
      norm_l="Lower",
      norm_u="Upper",
      units="Units"
    )
    .opt_vertical_padding(scale=0.4)
    .opt_align_table_header(align="left")
    .tab_options(heading_padding="10px")
    .tab_style(
        locations=loc.body(columns="norm_l"),
        style=style.borders(sides="left")
    )
    .opt_vertical_padding(scale=0.5)
)

illness

Fixes: #211
Partially addresses the .epic issue: #169

@github-actions github-actions bot temporarily deployed to pr-240 March 12, 2024 15:56 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 March 12, 2024 15:57 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 March 13, 2024 14:39 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 March 13, 2024 18:46 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 March 13, 2024 21:03 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 March 13, 2024 21:37 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 01:33 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 01:36 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 14:17 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 14:20 Destroyed
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.

Shoot sorry---I thought this PR had re-requested review, so made some comments 😅 . I'm going to submit the review, in case they're helpful, but sorry for jumping early on this :o

@@ -1489,6 +1489,415 @@ def test_fmt_image_path():
assert 'src="/a/b/c"' in strip_windows_drive(res)


def test_fmt_units():
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear what about fmt units is being tested here. Please...

  • test values to be formatted one by one
  • only test values that test / make clear something new about fmt_units

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now improved. Thanks!

Copy link
Collaborator

@machow machow May 24, 2024

Choose a reason for hiding this comment

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

Thanks for breaking this up. Do you mind expressing this using pytest.mark.parametrize, so that each is an individual test? This way, the CI will show at a glance which piece is failing, and we see the inputs / outputs close to eachother. (seems reasonable to keep the comments between each parameter case)

Currently, you have to look down at the specific assert, and then up to match the output with the input

Copy link
Member Author

Choose a reason for hiding this comment

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

Now done.

tests/test_helpers.py Outdated Show resolved Hide resolved
tests/test_helpers.py Show resolved Hide resolved
tests/test_helpers.py Show resolved Hide resolved
tests/test_helpers.py Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 14:47 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 16:02 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 16:23 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 16:28 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 16:32 Destroyed
@github-actions github-actions bot temporarily deployed to pr-240 May 24, 2024 16:51 Destroyed
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.

Add the fmt_units() method
3 participants