You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
btw for the reviewers @edelarua@ayogasekaram: 250+ line additions are simply comments, missing documentation, and missing tests. There were NO direct tests of the wrapping, hence imo the source of multiple problems and bugs downstream. Nonetheless, the wrapping algorithm now is entirely new and it has nothing shared with the old method. I kept wrap_txt for no other reason than the deprecation cycle. The indentation is as before removed and reinserted but now it is all self-contained in specific helper functions that have indentation checks that are a bit more formal than before. The indentation fixes were introduced by me when the wrapping went live and completely broke the indentation because base::strwrap destroys empty spaces. Now empty spaces are supported and they are destroyed only when trailing on a string that needs to be split because too long. Otherwise, they are kept as if they are a word (e.g. if you have " " (3 times \s), then it is considered as one word made of one white space). Failures related to \n need to be resolved in a separate PR as this is specifically related to wrap_string and toString refactoring while the \n solution needs to happen in matrix_form. In other words, here what it is missing from this refactoring that needs to touch other functions:
top left material needs to be better handled by matrix_form: indentation and wrapping are broken there and do not cooperate well with column names (which should be always top, I think - we can discuss this)
tf_wrap and max_width are overlapping parameters that should be merged into one (with if (!is.null(max_width)))
widths and colwidths are two parameters that do the same but there is no reason to keep them separate
more examples and vignette
Found downstream 3 exceptions to be handled more uniformly -> 1. ContentRows with no labels, 2. rtables::rtable() with no content does have mf_rinfo as null, 3. cases where input widths are 0 0 0
btw for the reviewers @edelarua @ayogasekaram: 250+ line additions are simply comments, missing documentation, and missing tests. There were NO direct tests of the wrapping, hence imo the source of multiple problems and bugs downstream. Nonetheless, the wrapping algorithm now is entirely new and it has nothing shared with the old method. I kept
wrap_txt
for no other reason than the deprecation cycle. The indentation is as before removed and reinserted but now it is all self-contained in specific helper functions that have indentation checks that are a bit more formal than before. The indentation fixes were introduced by me when the wrapping went live and completely broke the indentation becausebase::strwrap
destroys empty spaces. Now empty spaces are supported and they are destroyed only when trailing on a string that needs to be split because too long. Otherwise, they are kept as if they are a word (e.g. if you have" "
(3 times \s), then it is considered as one word made of one white space). Failures related to\n
need to be resolved in a separate PR as this is specifically related towrap_string
andtoString
refactoring while the\n
solution needs to happen inmatrix_form
. In other words, here what it is missing from this refactoring that needs to touch other functions:\n
and\t
, for example) need to be resolved before going intotoString
(i.e. inmatrix_form
)matrix_form
: indentation and wrapping are broken there and do not cooperate well with column names (which should be always top, I think - we can discuss this)tf_wrap
andmax_width
are overlapping parameters that should be merged into one (withif (!is.null(max_width))
)widths
andcolwidths
are two parameters that do the same but there is no reason to keep them separatertables::rtable()
with no content does havemf_rinfo
as null, 3. cases where input widths are 0 0 0Originally posted by @Melkiades in #203 (comment)
The text was updated successfully, but these errors were encountered: