stan-dev / shinystan

shinystan R package and ShinyStan GUI
https://mc-stan.org/shinystan
GNU General Public License v3.0
197 stars 51 forks source link

launch_shinystan() does not pass launch.browser argument to runApp #172

Closed danschrage closed 4 years ago

danschrage commented 4 years ago

Sometimes I want to launch shinystan without automatically opening a browser, even if I'm not in RStudio. This should be possible by passing launch.browser=FALSE to launch_shinystan(), which should pass ... along to runApp(). This doesn't work because the internal launch() function explicitly sets launch.browser to TRUE if not run in RStudio, which I don't think is the intention.

Reproducible example:

library(shinystan)
launch_shinystan(eight_schools, launch.browser=FALSE)

### Error in shiny::runApp(system.file("ShinyStan", package = "shinystan"),  : 
  formal argument "launch.browser" matched by multiple actual arguments

(Similarly, it ignores a call to set options(shiny.launch.browser=FALSE).)

If I look at the start of the launch() function in launch_shinystan.R, I see the issue:

launch <- function(sso, rstudio = FALSE, ...) {
  launch.browser <- if (!rstudio) 
    TRUE else getOption("shiny.launch.browser", interactive())

I'm not sure of the simplest way to avoid setting launch.browser explicitly while preserving your intended default behavior when running shinystan within RStudio. But it would be great if shinystan would at least respect the shiny.launch.browser option if it has been set by the user.

By changing the start of the launch() function to the following, it would preserve all current behavior unless the user has explicitly set shiny.launch.browser to FALSE using options():

launch <- function(sso, rstudio = FALSE, ...) {
  launch.browser <- if (!rstudio) 
    getOption("shiny.launch.browser", TRUE) else getOption("shiny.launch.browser", interactive())

If the user has not set shiny.launch.browser, then launch.browser will continue to default to TRUE if launch_shinystan() is not called from RStudio. But if a user has set that option, this will respect that choice.

I'll submit a pull request that implements this. Note that this won't fix the reproducible bug above, but it will provide the desired behavior if the user instead does this:

library(shinystan)
options(shiny.launch.browser=FALSE)
launch_shinystan(eight_schools)

Currently, the option is ignored by launch_shinystan().

danschrage commented 4 years ago

Oh, and if you choose my simple/partial fix that just ensure that options(shiny.launch.browser=FALSE) is respected, consider updating the documentation for launch_shinystan to make clear that ... will not pass launch.browser along to shiny::runApp. My reading of the documentation led me to expect that it would, which is what led me to this issue.

VeenDuco commented 4 years ago

Hello, thanks so much for pointing this out. I will take a closer look to see if we can create a full fix for this. I'll assign the issue to myself. Would you like to have the simple fix implemented in the meantime? I'm currently working on version 3.0 and could include your code in that version for now?

VeenDuco commented 4 years ago

I've added your quick-fix to the V3-Alpha version which you can get as follows:

if (!require("devtools")) {
  install.packages("devtools")
}
devtools::install_github("stan-dev/shinystan", ref = "v3-alpha", build_vignettes = TRUE)

Does this work for you?

danschrage commented 4 years ago

Yes, I tested v3-alpha, and it now works for my use case: If I explicitly set options(shiny.launch.browser=FALSE) then launch_shinystan() respects that and starts the server without launching a browser.

That works well enough for me, and I'll leave it to you to decide if it makes sense to also allow the user to pass launch.browser=FALSE directly as part of the call to launch_shinystan(). If not, I'd suggest adding a note in the documentation that this option is not passed by ... along to runApp() and to suggest setting the environment option instead.

Thanks for the fix!