Fmt prop bug #61
Fmt prop bug #61
Conversation
There was a problem hiding this comment.
I left comments based on the structure of changes you sent. But I would encourage you think about whether all of the changes are necessary. Specifically, does this issue that motivates this change only occur in tables? For example, if we can use fmt_prop() in inline text without issue, then the problem is only occurring in tables.
In that case, we don't necessarily need to change the fmt_prop() or fmt_prop_pct() functions. Those can continue to operate as is, keeping current code functional.
In that case, we would only need to edit the code that is used for tables (i.e., pad_prop()). I think we could pretty easily leave this function as is, and just add one line at the end that is something like:
if (output == "html") {
new_x <- stringr::str_replace_all(new_x, "<", "$\\lt$")
new_x <- stringr::str_replace_all(new_x, ">", "$\\gt$")
}
That way all the padding calculations are unchanged, and existing functionality will not be broken. We're only changing what needs to be changed in order to solve the problem.
| @@ -168,7 +168,7 @@ fmt_corr <- function(x, digits, output = NULL) { | |||
|
|
|||
| #' @export | |||
| #' @rdname formatting | |||
| fmt_prop <- function(x, digits, fmt_small = TRUE, keep_zero = FALSE) { | |||
| fmt_prop <- function(x, digits, fmt_small = TRUE, keep_zero = FALSE, output) { | |||
There was a problem hiding this comment.
output should have a default value. Probably output = NULL like in the fmt_corr() and fmt_minus() functions.
Should also check output using check_output()
| @@ -202,7 +204,7 @@ fmt_prop <- function(x, digits, fmt_small = TRUE, keep_zero = FALSE) { | |||
|
|
|||
| #' @export | |||
| #' @rdname formatting | |||
| fmt_prop_pct <- function(x, digits = 0, fmt_small = TRUE) { | |||
| fmt_prop_pct <- function(x, digits = 0, fmt_small = TRUE, output) { | |||
There was a problem hiding this comment.
Provide default value, check provided value.
| paste0(new_x, paste(rep("\\ ", pad), | ||
| collapse = "")), | ||
| TRUE ~ new_x) | ||
| if(is_html_output()) { |
There was a problem hiding this comment.
Why use is_html_output() here? In the function definition we allow the user to specify and output format. Shouldn't we respect that specification here?
| if ((any(stringr::str_detect(new_x, "lt")) | | ||
| any(stringr::str_detect(new_x, "gt"))) & | ||
| !(all(stringr::str_detect(new_x, "lt") | | ||
| stringr::str_detect(new_x, "gt")))) { |
There was a problem hiding this comment.
Can we compress this? e.g.,
any(stringr::str_detect(new_x, "lt|gt") &
!all(stringr::str_detect(new_x, "lt|gt")
| any(stringr::str_detect(new_x, "gt"))) & | ||
| !(all(stringr::str_detect(new_x, "lt") | | ||
| stringr::str_detect(new_x, "gt")))) { | ||
| pad <- ifelse(output == "latex", 4, 3) |
There was a problem hiding this comment.
This is currently in an if (is_html_output()), so this should only be for HTML, no?
I would recommend moving this line of code outside of the HTML/LaTeX if statements, then you just have the pad variable defined when you go into the if.
Closes #59
Replaces the
<symbol with$\\lt$and>with$\\gt$for the html versions of rendered tables.