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: missing values in fmt_number #317

Merged
merged 2 commits into from
Apr 30, 2024
Merged

fix: missing values in fmt_number #317

merged 2 commits into from
Apr 30, 2024

Conversation

jrycw
Copy link
Contributor

@jrycw jrycw commented Apr 25, 2024

Fix #314.

At first, I attempted to address this issue by utilizing the check from fmt_scientific to verify x:

if is_na(self._tbl_data, x):
    return x

However, this caused a failure in tests/test_export.py::test_html_string_generated. Consequently, I opted to directly check whether x is None or not.

from great_tables import GT, exibble
import polars as pl

exibble_pl = pl.from_pandas(exibble)

GT(exibble_pl).fmt_number(columns="num")

image

@codecov-commenter
Copy link

codecov-commenter commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.46%. Comparing base (38033ce) to head (640ab52).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #317   +/-   ##
=======================================
  Coverage   82.45%   82.46%           
=======================================
  Files          41       41           
  Lines        4258     4260    +2     
=======================================
+ Hits         3511     3513    +2     
  Misses        747      747           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@machow
Copy link
Collaborator

machow commented Apr 29, 2024

shoot... thanks for looking into this. I looked at a diff of the snapshot, and it seems okay to use the same is_na() guard is this function, since it's behavior will match the others. Hope it's okay, I just added to this PR!

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@rich-iannone
Copy link
Member

Thank you @jrycw !

@machow machow merged commit 2917c5b into posit-dev:main Apr 30, 2024
9 checks passed
@jrycw jrycw deleted the fix-314 branch April 30, 2024 13:50
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.

The GT.fmt_number() method fails when formatting includes missing values in Polars DataFrames
4 participants