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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
tests/test_formats.py
Outdated
@@ -1489,6 +1489,415 @@ def test_fmt_image_path(): | |||
assert 'src="/a/b/c"' in strip_windows_drive(res) | |||
|
|||
|
|||
def test_fmt_units(): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now done.
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 thefmt_*()
, 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 theillness
dataset. It so happens that theunits
column has strings in units notation, so, we just need to point this method to that column:Fixes: #211
Partially addresses the
.epic
issue: #169