Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve run_tutorial() #601

Merged
merged 31 commits into from
Dec 29, 2021
Merged

Improve run_tutorial() #601

merged 31 commits into from
Dec 29, 2021

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Oct 15, 2021

Brings a few new features to run_tutorial():

  • Allow name to take a path to local tutorial directory. The name must be an existing directory and package must be NULL. Since run_tutorial() manages the tutorial directory and uses the dir argument of rmarkdown::run(), we pre-emptively thorw an error if there is more than one .Rmd in the directory and none are named index.Rmd.

  • Add arg clean to run_tutorial(), cleanly re-render the tutorial when TRUE. We follow the same logic used in validating the tutorial directory to identify the tutorial .Rmd file, since rmarkdown::shiny_prerendered_clean() requires a specific file rather than a directory.

  • Launch tutorial in viewer pane by default, if possible. It's not currently possible to open tutorials in the tutorial pane interactively, but by giving launch.browser a function, we have a place to modify this in the future.

Fixes #600
Fixes #333

Copy link
Contributor

@rossellhayes rossellhayes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a small typo!

R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
tests/testthat/test-run.R Outdated Show resolved Hide resolved
@gadenbuie
Copy link
Member Author

Update: I also added support, turned on by default, for running a tutorial as a background RStudio job. The tutorial opens wherever shiny apps open by default, using the shiny.launch.browser option.

In theory, we could also open the tutorial in the Tutorials pane, but since there isn't a public API for stopping Rstudio jobs, we'd end up having to build too much infrastructure to achieve parity with the built in tutorial running methods. That's another reason why I decided not to go further; the RStudio tutorial pane depends on run_tutorial() and I'd prefer to avoid a cyclical dependency. The new default for run_tutorial() is to use an RStudio job, but it can be disabled by setting as_rstudio_job = FALSE and it, by default, won't try to run as a job in non-interactive environments (like other RStudio jobs).

We don't directly need this version in learnr code but we might as well encourage learnr users to have the appropriate version
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes.

  • Remove multi-Rmd-file logic warning in available_tutorials_for_package()
  • Add Breaking Change news item about shiny_args now being after ...

Otherwise LGTM!

Copy link
Member Author

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad I let this sit for a little bit. Coming back to look at it again, I think the restriction that name be a directory when referring to a local file feels too arbitrary.

I stuck with the directory approach because we move the tutorial directory to a temporary location if we don't have write access where the tutorial is stored. That logic makes sense when the tutorial is hosted in an R package, which may be stored in a location that's inaccessible to the end user. But it doesn't make sense for a local file.

I'm going to update the PR to accept a specific file in name. If a file path is provided but the directory isn't writeable, we'll throw an error rather than copy the directory.

@chendaniely

This comment has been minimized.

Copy link
Contributor

@rossellhayes rossellhayes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I just caught a few typos and one potential small improvement

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
R/run.R Outdated Show resolved Hide resolved
Co-authored-by: Alex Rossell Hayes <44556601+rossellhayes@users.noreply.github.com>
@gadenbuie

This comment has been minimized.

@gadenbuie gadenbuie merged commit 7f26e84 into main Dec 29, 2021
@gadenbuie gadenbuie deleted the improve-run-tutorial branch December 29, 2021 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants