Closed donyunardi closed 8 months ago
Also make sure
Title
is not duplicated in Package Description
in DESCRIPTION file (e.g. this happens in teal.slice currently)Title
and Description
fields of DESCRIPTION file are quoted with '
:::
in examplesteal.*
mentions are lower-cased and quoted/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 releasedFollowing-up on @chlebowa comment on applying https://github.com/insightsengineering/teal.slice/pull/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?
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.
Besides {teal} that you already gave a quick look to the other packages
Non-exaustive look:
teal.code
shiny
and magrittr
({r, eval=requireNamespace("shiny") && requireNamespace("magrittr")}
)requireNamespace
, tinytex
is apparently not used{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
andmagrittr
({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.
{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 ofr-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
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.
{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 renderI 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>)
?
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
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
tinytex
was introduced 2 years ago in teal.reporter
during this commit https://github.com/insightsengineering/teal.reporter/commit/aa7960887b6b373695286077b598cd9700cfb0c7
Maybe we can double check if rmarkdown
does not need that anymore to render documents
I don't think it's needed. tinytex exists to help installing portable versions of LaTeX
. Rendering is done with system pdflatex
engine.
Isn't it needed for proper installation then?
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.
Can you test if pdf
reports render properly on a naive installation w/o tinytex
?
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
Agree with @m7pr
Pre-release activities are done. Closing.
Summary
Please carry out these tasks in preparation for the CRAN release:
urlchecker::url_check()
to identify broken links and fix@example
tag, make sure it runs, fix if otherwise@return
tag to document the return value\dontrun
tag, replace withif(interactive())
if neededR CMD check --as-cran
make sure everything passThis needs to be done for the following repos:
Please assign your name to the checkbox and perform the pre-release activities. Create PR if fix is needed.