Closes #2563 no_list_columns: add check to avoid list columns

[bundfussr]

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the main branch until you have checked off each task.

• Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
• If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• Review the Cheat Sheet. Make any required updates to it by editing the file inst/cheatsheet/admiral_cheatsheet.pptx and re-upload a PDF and a PNG version of it to the same folder. (The PNG version can be created by taking a screenshot of the PDF version.)
• Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Address any updates needed for vignettes and/or templates
• Update NEWS.md under the header # admiral (development version) if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers). A Developer Notes section is available in NEWS.md for tracking developer-facing issues.
• Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
• Address or fix all lintr warnings and errors - lintr::lint_package()
• Run R CMD check locally and address all errors and warnings - devtools::check()
• Link the issue in the Development Section on the right hand side.
• Address all merge conflicts and resolve appropriately
• Pat yourself on the back for a job well done! Much love to your accomplishment!

[github-actions]

Package	Line Rate	Health
admiral	98%	✔
Summary	98% (5186 / 5299)	✔

[jimrothstein]

I'm a little over my head ...

• git cloned branch 2563 no_list_columns
• Manually knitr occds.Rmd - looks fine, tables nicely formatted.
  -This line throws NO ERROR

derive_vars_atc(cm, dataset_facm = facm, id_vars = exprs(FAGRPID))

• BUT, Build failed, with this below:

   --- re-building ‘occds.Rmd’ using rmarkdown
   --- finished re-building ‘occds.Rmd’
   
   --- re-building ‘pk_adnca.Rmd’ using rmarkdown
   --- finished re-building ‘pk_adnca.Rmd’
   
   --- re-building ‘queries_dataset.Rmd’ using rmarkdown
   --- finished re-building ‘queries_dataset.Rmd’
   
   --- re-building ‘questionnaires.Rmd’ using rmarkdown
   
   Quitting from lines 177-198 [unnamed-chunk-10] (questionnaires.Rmd)
   Error: processing vignette 'questionnaires.Rmd' failed with diagnostics:
   ℹ In argument: `AVAL = `%>%`(...)`.
   ℹ In group 1: `STUDYID = "STUDYX"`, `USUBJID = "P0001"`, `AVISIT = "BASELINE"`,
     `ADT = 2012-11-16`, `ADY = 1`, `TRTSDT = 2012-11-16`, `DTHCAUS = NA`.
   Caused by error in `assert_numeric_vector()`:
   ! unused argument (length = 2)
   --- failed re-building ‘questionnaires.Rmd’
   
   --- re-building ‘visits_periods.Rmd’ using rmarkdown
   [WARNING] Could not fetch resource https://raw.githubusercontent.com/pharmaverse/admiral/main/inst/visit_periods/dummy_study_design.png: HttpExceptionRequest Request {
       host                 = "raw.githubusercontent.com"
       port                 = 443
       secure               = True
       requestHeaders       = []
       path                 = "/pharmaverse/admiral/main/inst/visit_periods/dummy_study_design.png"
       queryString          = ""
       method               = "GET"
       proxy                = Nothing
       rawBody              = False
       redirectCount        = 10
       responseTimeout      = ResponseTimeoutDefault
       requestVersion       = HTTP/1.1
       proxySecureMode      = ProxySecureWithConnect
     }
      ConnectionTimeout
   --- finished re-building ‘visits_periods.Rmd’
   
   SUMMARY: processing the following file failed:
     ‘questionnaires.Rmd’
   
   Error: Vignette re-building failed.
   Execution halted
Error in `(function (command = NULL, args = character(), error_on_status = TRUE, …`:
! System command 'R' failed
---
Exit status: 1
stdout & stderr: <printed>
---
Backtrace:
 1. devtools::build()
 2. pkgbuild::build(path = pkg, dest_path = path, binary = binary, …
 3. pkgbuild:::withr_with_makevars(compiler_flags(debug = FALSE), { …
 4. pkgbuild:::withr_with_envvar(c(R_MAKEVARS_USER = makevars_file), { …
 5. base::force(code)
 6. base::force(code)
 7. pkgbuild:::withr_with_temp_libpaths(rcmd_build_tools(options$cmd, c(options$path, …
 8. base::force(code)
 9. pkgbuild::rcmd_build_tools(options$cmd, c(options$path, options$args), …
10. pkgbuild::with_build_tools({ …
11. base::withCallingHandlers(callr::rcmd_safe(..., env = env, spinner = FALSE, …
12. callr::rcmd_safe(..., env = env, spinner = FALSE, show = FALSE, …
13. callr:::run_r(options)
14. base::with(options, with_envvar(env, do.call(processx::run, c(list(bin, …
15. base::with.default(options, with_envvar(env, do.call(processx::run, …
16. base::eval(substitute(expr), data, enclos = parent.frame())
17. base::eval(substitute(expr), data, enclos = parent.frame())
18. callr:::with_envvar(env, do.call(processx::run, c(list(bin, args = real_cmdargs, …
19. base::force(code)
20. base::do.call(processx::run, c(list(bin, args = real_cmdargs, stdout_line_callback = real_callback(stdout), …
21. (function (command = NULL, args = character(), error_on_status = TRUE, …
22. base::throw(new_process_error(res, call = sys.call(), echo = echo, …
23. | base::signalCondition(cond)
24. (function (e) …
25. asNamespace("callr")$err$throw(e)
Execution halted

Exited with status 1.

[bms63]

@jimrothstein this is due to having an older version of admiraldev installed.

you need to install latest from github by using devtools::install_github("pharmaverse/admiraldev") and you will

be sure to restart and verify that you are using latest dev version 1.1.0.9006

[ddsjoberg]

@jimrothstein this is due to having an older version of admiraldev installed.

you need to install latest from github by using devtools::install_github("pharmaverse/admiraldev") and you will

be sure to restart and verify that you are using latest dev version 1.1.0.9006

When we depend on a dev version of a package, we should also indicated this in our DESCRIPTION file FYI

Imports:
    admiraldev (>= 1.1.0.9006),

It used to be the case, that Remotes: would re-install all pkgs listed in Remotes upon installation of the pkg, but that is no longer the case after pak replaced remotes for GH intallations.

[jimrothstein]

No problems or errors:

• With packageVersion("admiraldev")
  [1] ‘1.1.0.9007’
• fresh git clone to local machine
• builds, tests fine.
• occds vignette looks great (via knitr)

@ddsjoberg

Just fyi,
at this moment, main branch still shows:

Imports:
admiraldev (>= 1.1.0),

[bundfussr]

I have updated the required admiraldev version in DESCRIPTION. I hope this will reminds us of updating to admiraldev (>= 1.2.0) before we submit to CRAN. @bms63 , do you have this on your checklist?

I wonder if we should add a function/addin to pharmaverse4devs which installs the main versions of the upstream dependencies. Optionally the latest released version of the upstream dependencies could be installed (for hotfixes).

@bms63 , what do you think?

[bms63]

I added this check to my release checklist!! That is a good one!!!

.

If we have Remotes in DESCRIPTION for admiral and used devtools::install_dev_deps(dependencies = TRUE) would that grab the latest dev version?

[ddsjoberg]

I hope this will reminds us of updating to admiraldev (>= 1.2.0) before we submit to CRAN.

FYI if we have a Remotes: in the DESCRIPTION file when we submit to CRAN, we'll get auto-rejected (which happens typically within 20 minutes of submission).

[bms63]

I am happy! :) @jeffreyad @jimrothstein @ddsjoberg any other thoughts? if not lets get this merged in before Stefan leaves for the day (noon US EST)

[bms63]

@bundfussr this is popping up in my branch now

[bundfussr]

@bundfussr this is popping up in my branch now

@bms63 , good catch! I missed to add the snapshot. I've added it in #2625.

Unfortunately, R-CMD check doesn't fail if warnings occur in the unit tests. I couldn't find an option to change this behaviour. @ddsjoberg , any ideas?

[ddsjoberg]

@bundfussr if we used the standard R CMD Check workflow from Posit, then default is to error when warnings are present. Otherwise, we'd need to ask the IDR team whether it's possible in their current workflows. I am in favor of replacing this workflow with the standard one: https://github.com/pharmaverse/admiral/blob/main/.github/workflows/R-CMD-check.yaml

I don't like how the one we have only checks against dev versions. I would much prefer we use the DESCRIPTION file to specify if we depend on dev versions of packages. Also, it makes it difficult for someone who installs the package from github to get a working version when we do indeed depend on a dev version of, for example, admiraldev, because in the past we've forgotten to update the DESCRIPTION file with this information (which is how all the installation functions identify dep packages needed). Anyway, I think I am a broken record on this point 😝

[bundfussr]

@bundfussr if we used the standard R CMD Check workflow from Posit, then default is to error when warnings are present. Otherwise, we'd need to ask the IDR team whether it's possible in their current workflows. I am in favor of replacing this workflow with the standard one: https://github.com/pharmaverse/admiral/blob/main/.github/workflows/R-CMD-check.yaml

I don't like how the one we have only checks against dev versions. I would much prefer we use the DESCRIPTION file to specify if we depend on dev versions of packages. Also, it makes it difficult for someone who installs the package from github to get a working version when we do indeed depend on a dev version of, for example, admiraldev, because in the past we've forgotten to update the DESCRIPTION file with this information (which is how all the installation functions identify dep packages needed). Anyway, I think I am a broken record on this point 😝

@ddsjoberg , all our R CMD Check workflows fail if R CMD check issues a warning. The problem is that a warning in a unit test doesn't trigger a warning in the R CMD check. Consider for example https://github.com/pharmaverse/admiral/actions/runs/12655315803/job/35265315719. In R CMD check the unit tests pass:

But one of the unit tests issues a warning because I missed to commit one of the snapshots:

Do you know if this behavior can be changed?

[bundfussr]

We could use

test_check("admiral", stop_on_warning = TRUE)

in tests/testthat.R.
Then a warning in one of the unit tests triggers a failure in R CMD check. I think this is better than ignoring warnings in unit tests.

@bms63 , @ddsjoberg , what do you think?

[bms63]

We could use

test_check("admiral", stop_on_warning = TRUE)

in tests/testthat.R. Then a warning in one of the unit tests triggers a failure in R CMD check. I think this is better than ignoring warnings in unit tests.

@bms63 , @ddsjoberg , what do you think?

that feels sensible and nice to use already existing tools!!

[ddsjoberg]

looks great!

[bundfussr]

Handling of warnings in unit tests was changed in #2628.