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

main_footer is not indented if table_inset is not 0 and tf_wrap = FALSE #705

Closed
clarkliming opened this issue Aug 9, 2023 · 10 comments
Closed

Comments

@clarkliming
Copy link
Contributor

clarkliming commented Aug 9, 2023

a <- basic_table() %>%
  analyze("Sepal.Length") %>%
  build_table(iris)

main_footer(a) <- "very very very very very very very very very very\nvery very very very very very very very long"
table_inset(a) <- 2L
cat(export_as_txt(a, tf_wrap = F))

it looks strange that only the first line is indented

         all obs
  ——————————————
  Mean    5.84  
  ——————————————

  very very very very very very very very very very
very very very very very very very very long

sessionInfo

R version 4.2.2 Patched (2022-11-10 r83330)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.5 LTS

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0
LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] rtables_0.6.2.9001    magrittr_2.0.3        formatters_0.5.1.9001

loaded via a namespace (and not attached):
 [1] compiler_4.2.2  backports_1.4.1 fastmap_1.1.0   cli_3.6.0      
 [5] tools_4.2.2     htmltools_0.5.4 grid_4.2.2      checkmate_2.1.0
 [9] digest_0.6.31   jsonlite_1.8.4  rlang_1.0.6    

@shajoezhu
Copy link
Collaborator

Hi @clarkliming , I was wondering if the expected outcome is as the following?

         all obs
  ——————————————
  Mean    5.84  
  ——————————————

  very very very very very very very very very very
  very very very very very very very very long

@clarkliming
Copy link
Contributor Author

Hi @clarkliming , I was wondering if the expected outcome is as the following?

         all obs
  ——————————————
  Mean    5.84  
  ——————————————

  very very very very very very very very very very
  very very very very very very very very long

yes

@gmbecker
Copy link
Contributor

@clarkliming This code tells rtables to not wrap the footer, and then manually put in a \n followed directly by a character. At first blush this seems like rtables has done what you have asked it to do: No wrapping by rtables is happening here at all. rtables considers the footer to be a single string, so your string is catted out, the first part is indented, and then a newline is emitted and then some characters are emited, but that is done by cat, which doesnt care about rtables' concept of table_indent.

We also provide a max-title/footer width, which behaves with table inset, so its unclear why there would be code that is turnign that off, and then trying to do it itself.

@clarkliming
Copy link
Contributor Author

No automatic wrapping because it usually does not look good, information are mixed and is not clear. So manual wrapping is needed here.

But when wrapping manually, it is ridiculous to not indent each line with the table_inset. So if it has to be a single line of text to disable "tf_wrap" why bother provide this control we can tell if the text contains line breaks or not.

And if the table_inset do not deal with the footer,does it means that everytime i change the table inset i have to update the footnotes? That is even more ridiculous.

@gmbecker
Copy link
Contributor

gmbecker commented Aug 16, 2023

First off, I do not appreciate the tone. Especially, since if you had bothered talk to me or otherwise report the difficulties you were having with wrapping, you would have known that this is a fully solved problem and the work around you're having trouble with is not needed, because main_footer accepts a character vector and each element of it is handled and wrapped separately during rendering, and that has always been the case. So no, manual wrapping via embedded newlines is not needed there.

lyt <- basic_table(main_footer = c("Footer line 1", "Footer line 2"),
                   inset = 3) %>%
    split_cols_by("ARM") %>%
    split_rows_by("STRATA1") %>%
    analyze("AGE")

tbl <- build_table(lyt, DM)
cat(export_as_txt(tbl, max_width = 9, tf_wrap = TRUE))
            A: Drug X   B: Placebo   C: Combination
   ————————————————————————————————————————————————
   A                                               
     Mean     32.53       32.30          35.76     
   B                                               
     Mean     35.46       32.42          34.39     
   C                                               
     Mean     36.34       34.45          33.54     
   ————————————————————————————————————————————————

   Footer
   line 1
   Footer
   line 2

@clarkliming
Copy link
Contributor Author

sorry about the tone. If the "main_footer" can be character vector, it solve the issue, but the documentation here may need to be updated.

https://github.com/insightsengineering/formatters/blob/ebd84a167a1699ecf026e2267f4ecd3d7487084a/R/generics.R#L370

and running the following code gives

lyt <- basic_table() %>%
  split_cols_by("Species") %>%
  analyze("Sepal.Length", afun = function(x) {
  list(
    "mean (sd)" = rcell(c(mean(x), sd(x)), format = "xx.xx (xx.xx)"),
    "range" = diff(range(x))
  )
})
tbl <- build_table(lyt, iris)
tbl

main_title(tbl) <- "this is main title"
main_footer(tbl) <- c("this is main footer 1", "this is main footer 2 that is super long")
subtitles(tbl) <- c("this is subtitle", "this is subtitle2")
prov_footer(tbl) <- c("this is prov footer1", 'this is prov footer 2 that is super long')

table_inset(tbl) <- 2
export_as_txt(tbl, max_width = 20, tf_wrap = T) %>% cat
Warning message:
In length(prfoot) && nzchar(prfoot) :
  'length(x) = 2 > 1' in coercion to 'logical(1)'

@gmbecker
Copy link
Contributor

gmbecker commented Aug 23, 2023

So the documentation is correct, but a bit subtle, I don't object to it being more explicit.

For all argument descriptions in formatters/rtables, the first element of the argument description is the type of object it takes.

For basic_table, you can see that for title, you can see it starts with "character(1)", this indicates a character vector of length 1 (I have already brought up with the team that this should be expanded to match the other entries, but what level of priority that change is given is out of my control). Meanwhile, subtitles, main_footer, and prov_footer are documented as "character", which means a character vector of any length.

That last warning is a bug. (it will actually be an error in some versions of R). I have fixed it in the above pull request for formatters, though I don't know if it will get into this release. Thats up to @shajoezhu and the CRAN team

Note that for versions of R where it is a warning, the warning is annoying and wrong, but provided that the first element of prov_footer is not the empty string, the actual behavior (ie the text output by export_as_txt) should be correct.

@clarkliming
Copy link
Contributor Author

clarkliming commented Aug 24, 2023

the document is not correct in formatters::main_footer (it says main_footer is a character scalar, instead of a vector)

#' @return a character scalar (`main_title`, `main_footer`), or
#' vector of length zero or more (`subtitles`, `page_titles`,
#' `prov_footer`) containing the relevant title/footer contents

@gmbecker
Copy link
Contributor

the document is not correct in formatters::main_footer (it says main_footer is a character scalar, instead of a vector)

#' @return a character scalar (main_title, main_footer), or #' vector of length zero or more (subtitles, page_titles, #' prov_footer) containing the relevant title/footer contents

Correct, this is a documentation bug that should be fixed @Melkiades @ayogasekaram

@Melkiades
Copy link
Contributor

I think we can close this as the fix for documentation is tracked by another issue

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

No branches or pull requests

4 participants