-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 trailing "." when using format_sigfig
, add functionality for multiple values
#1067
Conversation
Code Coverage Summary
Diff against main
Results for commit: 151cd13 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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 @edelarua !
format_sigfig
format_sigfig
, add functionality for multiple values
Notes on implementation:
|
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.
Great job Emily! Thanks for addressing all my doubts :)
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.
@edelarua this is looking a lot better but I think we need to be careful about which formats are supported. I suggest to either restrict the format list more explicitly and/or add more tests to verity that all formats in list_valid_format_labels()
are supported:
Here's 2 cases to think about, both seem to be bugs in format_value
:
(1) this gives an error:
num1 <- c(0.1239)
z <- format_sigfig(3, format = "xx%" )
z(num1)
>Error in x * 100 : non-numeric argument to binary operator
(2) This gives the wrong result "<0.0001"
num2 <- c(1234)
z <- format_sigfig(3, format = "x.xxxx | (<0.0001)" )
z(num2)
@anajens I have specified the formats that work in the documentation and added a check in the function. |
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, this is much safer 😅
Closes #1066