insightsengineering / teal.modules.general

General Purpose Teal Modules
https://insightsengineering.github.io/teal.modules.general/
Other
9 stars 13 forks source link

`shinyApp` chunk in vignettes only runs if is in interactive mode #741

Closed averissimo closed 6 months ago

averissimo commented 7 months ago

Pull Request

Changes description

How to test

github-actions[bot] commented 7 months ago

Unit Tests Summary

ā€‡ā€‡1 filesā€„ā€ƒā€‡22 suitesā€„ā€ƒā€‚10m 42s :stopwatch: 147 testsā€ƒ147 :white_check_mark:ā€ƒ0 :zzz:ā€ƒ0 :x: 478 runsā€Šā€ƒ478 :white_check_mark:ā€ƒ0 :zzz:ā€ƒ0 :x:

Results for commit 67392cd8.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 7 months ago

Unit Test Performance Difference

Test Suite $Status$ Time on main $Ā±Time$ $Ā±Tests$ $Ā±Skipped$ $Ā±Failures$ $Ā±Errors$
shinytest2-tm_variable_browser šŸ’” $47.98$ $+1.78$ $0$ $0$ $0$ $0$

Results for commit d8739f84d1aab8821f1b7a2f8cfd61175b12c6f6

ā™»ļø This comment has been updated with latest results.

pawelru commented 7 months ago

This looks good. I two comments:

averissimo commented 7 months ago

runtime: shiny

  • can you please check whether runtime: shiny is required. I actually have no idea what it does

BLUF: It's not necessary and it does nothing in this case. I'm removing it.

Some context It does not allow a ShinyApp though. Hence why it does nothing here. When this runtime is set to `shiny` it allows for shiny elements on the rendered markdown (but not on `pkgdown` sites) > (from docs) The runtime target for rendering. The static option produces output intended for static files; shiny produces output suitable for use in a Shiny document (see run). The default, auto, allows the runtime target specified in the YAML metadata to take precedence, and renders for a static runtime target otherwise. ![image](https://github.com/insightsengineering/teal.modules.general/assets/211358/33fa18ca-2284-46af-871b-79e0a4ed0f3d)

rlang::is_interactive()

  • (assuming we already depend on rlang) have you considered rlang::is_interactive(). This is the function I have discovered quite recently and I don't know whether this is really better than the one from base.

No practical benefits here, but rlang::is_interactive() is technically better as it will detect the right context . Unlike, base::interactive that only detects if there is an interactive session.

When can it be helpful?

  1. Forcing the result to be either value (via global option)
  2. Manual running knitr or testthat in a console
    • rlang::is_interactive() as FALSE, where base version is TRUE

image

image

averissimo commented 7 months ago

The {verdepcheck} GitHub workflow ran successfully

This PR should also fix the GRAN and r-universe build fails.