Pre-release activities

[donyunardi]

Summary

Please carry out these tasks in preparation for the CRAN release:

1. Review and update:
   • README.md (check example code)
   • NEWS.md
2. Run urlchecker::url_check() to identify broken links and fix
3. Review functions:
   • @example tag, make sure it runs, fix if otherwise
   • Make sure functions has @return tag to document the return value
   • no \dontrun tag, replace with if(interactive()) if needed
4. Sanity check of all vignettes, make sure there is no typo, no wrong format, etc.
5. Run R CMD check --as-cran make sure everything pass

This needs to be done for the following repos:

• Pre-release activities teal.code#189 @kartikeyakirar
• Pre-release activities teal.data#240 @kartikeyakirar
• Pre-release activities teal.slice#506 @kartikeyakirar
• Pre-release activities teal#1021 @kartikeyakirar

Please assign your name to the checkbox and perform the pre-release activities.
Create PR if fix is needed.

[m7pr]

Also make sure

• Package Title is not duplicated in Package Description in DESCRIPTION file (e.g. this happens in teal.slice currently)
• All package names in Title and Description fields of DESCRIPTION file are quoted with '
• You have checked the Package Release Template https://github.com/insightsengineering/teal.reporter/pull/205/files
• Make sure there are no ::: in examples
• Make sure all teal.* mentions are lower-cased and quoted
• Make Sure inst/WORDLIST is minimalized
• Make sure non-exported functions do not have examples
• Make sure each link to our documentation hosted with pkgdown on github pages do not have /main/ in the address but it has /latest/ instead, so we always expose the documentation of the latest release and not what's currently on main branch but not yet released

[averissimo]

Following-up on @chlebowa comment on applying insightsengineering/teal.slice#518 to all core packages (graceful handling of Suggested packages and skipping tests that use them)

(comment) I suppose we will do this in all package, not just teal.slice, correct?

Should we do this, and if so should it be part of the pre-release activities or track it in a different tasks on this repo?

[chlebowa]

I know for a fact that it is an issue in teal but I haven't looked at other packages yet. If it's limited to teal, I say we do it now.

[averissimo]

Besides {teal} that you already gave a quick look to the other packages

Non-exaustive look:

• {teal.transform}
  • has an examples that depends on Suggested teal.code
• {teal.code}
  • Vignette depends on Suggested shiny and magrittr ({r, eval=requireNamespace("shiny") && requireNamespace("magrittr")})
• {teal.data} & {teal.logger}
  • no action required
• {teal.widgets}
  • Examples & tests with a bunch (7) of Suggested packages (DT, lattice, magrittr, png, shinytest2, shinyvalidate, withr)
• {teal.reporter}
  • with some examples & tests (DT, formatR, lattice, png, rtables, tinytex)
  • {formatR} is already protected by requireNamespace, tinytex is apparently not used

[chlebowa]

{teal.transform}

• has an examples that depends on Suggested teal.code

Do we need them? Can we drop them? I have little certainty about teal.transform.

{teal.code}

• Vignette depends on Suggested shiny and magrittr ({r, eval=requireNamespace("shiny") && requireNamespace("magrittr")})

I actually remember wondering if we should just add eval=FALSE to vignette chunks and drop those dependencies.

lattice is installed by default as part of r-base, I believe.
tinytex is probably necessary to render .pdf files.

[averissimo]

{teal.transform}

• has an examples that depends on Suggested teal.code

Do we need them? Can we drop them? I have little certainty about teal.transform.

We may make due without them, but from a first look it seems useful for a "best practices" example.

{teal.transform} will be refactored soon, so a simple @examplesIf requireNamespace("teal.code") will push the question "Do we need it?" down the line

lattice is installed by default as part of r-base, I believe.

It has a "Recommended" priority, but it probably still needs to be declared to be used

tinytex is probably necessary to render .pdf files.

I found it weird that there's no call using it anywhere in the codebase

I actually remember wondering if we should just add eval=FALSE to vignette chunks and drop those dependencies.

Not sure if we remove the Suggestted packages even if we use eval=FALSE. My intuition is that any piece of code of the package should run if all packages are installed.

[chlebowa]

{teal.transform} will be refactored soon, so a simple @examplesIf requireNamespace("teal.code") will push the question "Do we need it?" down the line

I think this is acceptable.

tinytex is probably necessary to render .pdf files.

I found it weird that there's no call using it anywhere in the codebase

I don't recall ever calling tinytex explicitly but I do recall that my pdf exports only started working when I installed it. I never had the time to investigate fully.

I think chunks with eval=FALSE are not evaluated when the vignette is built.

But then I think the reason I left them in is we want that code to be run to detect potential bugs. The app isn't run but still.

And we couldn't replace %>% with |> because that would involve depending on R => 4.0.0.

Maybe we should use eval = requireNamespace(<pkg>)?

[shajoezhu]

And we couldn't replace %>% with |> because that would involve depending on R => 4.0.0.

This will require coordination across all nest packages, we need to align on a date I think

[chlebowa]

And we couldn't replace %>% with |> because that would involve depending on R => 4.0.0.

This will require coordination across all nest packages, we need to align on a date I think

There's no rush, however since CEDAR runs the development version of R we shouldn't fall too far behind or we risk some technological debt. Take a look at this: https://github.com/insightsengineering/coredev-tasks/issues/490

[m7pr]

tinytex was introduced 2 years ago in teal.reporter during this commit insightsengineering/teal.reporter@aa79608
Maybe we can double check if rmarkdown does not need that anymore to render documents

[m7pr]

I don't think it's needed. tinytex exists to help installing portable versions of LaTeX. Rendering is done with system pdflatex engine.

[chlebowa]

Isn't it needed for proper installation then?

[m7pr]

But where this installation happens? If on the end user machine, then (s)he needs to run tinytex::install_tinytex() anyway.
If on one of RStudio's docker containers, then I believe it's not needed since LaTeX distribution (TeX Live) is installed already with a system library https://github.com/rocker-org/rocker-versioned/blob/master/r-ver/3.6.3.Dockerfile#L70

It's not a big deal since tinytext is in Suggests, so will not be installed anyway by the default install.packages call.

[chlebowa]

Can you test if pdf reports render properly on a naive installation w/o tinytex?

[m7pr]

I don't think the test is needed. If you run rmarkdown::render(output_format = 'pdf_document') and you have any distribution of LaTeX on your machine (MiKTeX on Windows, MacTeX on macOS or TeX Live / TinyTeX for Linux) it will render. If you do not have LaTeX you will get a warning:

Error: LaTeX failed to compile test.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips.
In addition: Warning message:
In system2(..., stdout = if (use_file_stdout()) f1 else FALSE, stderr = f2) :
  '"pdflatex"' not found
Execution halted

No LaTeX installation detected (LaTeX is required to create PDF output). You should install a LaTeX distribution for your platform: https://www.latex-project.org/get/

  If you are not sure, you may install TinyTeX in R: tinytex::install_tinytex()

  Otherwise consider MiKTeX on Windows - http://miktex.org

  MacTeX on macOS - https://tug.org/mactex/
  (NOTE: Download with Safari rather than Chrome _strongly_ recommended)

  Linux: Use system package manager

Installed tinytex R package doesn't guarantee you have required LaTeX distribution. Only running tinytex::install_tinytex() in R does that. So if tinytex::install_tinytex() is not called for you, then tinytex is in Suggests doesn't fix missing LaTeX if you don't have it on the machine that tries to render the document. On another hand, if you already have LaTeX distribution, you don't need tinytext for anything.

LASTLY, and the most important: rmarkdown depends on tinytex so the first time you will try to run knit in RStudio, or rmarkdown::render you will get tinytex installed for you, so it does not need to be in our package. https://cran.r-project.org/web/packages/rmarkdown/index.html

[averissimo]

Agree with @m7pr

[donyunardi]

Pre-release activities are done.
Closing.