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

safe expression evaluation with callr #174

Merged
merged 8 commits into from
Sep 6, 2018
Merged

Conversation

schloerke
Copy link
Collaborator

@schloerke schloerke commented Jul 27, 2018

Address scoping issues within #162

Pre rendered documents can now be rendered in a clean environment using:

learnr::safe(rmarkdown::render(FILE))

Tutorials should be able to be run in a clean environment using:

learnr::safe(learnr::run_tutorial(TUTORIAL))

This is done as rmarkdown has no intent on adding a new session for compilation. rstudio/rmarkdown#1204

@schloerke
Copy link
Collaborator Author

schloerke commented Jul 30, 2018

  • NEWS item
  • test

R/run.R Outdated
})
})
}


safe_env <- function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be helpful to have a comment of why this function exists (because it's the same as the default for callr::r except that it allows opening a browser).

@schloerke schloerke changed the title safe render and safe run [WIP] safe expression evaluation with callr Aug 1, 2018
@schloerke
Copy link
Collaborator Author

Waiting to see response from r-lib/callr#72 about safe being adopted within callr

@schloerke
Copy link
Collaborator Author

callr does not want to include the safe function. Will keep within learnr

R/run.R Outdated
#' @examples
#' \dontrun{
#' # Direct usage
#' safe({learnr::run_tutorial("slidy", package = "learnr")})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having { } around code that's used as an expression is good for some contexts (e.g., shiny and profvis), but in this case, it might just be confusing for ordinary users of learnr.

For this example, I think you also don't want learnr::, to keep things simple for users.

Here's what you'd end up with after those two changes. This will be easier for new users to comprehend:

safe(run_tutorial("slidy", package = "learnr"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem, I can remove the brackets.

To get the code to work, I'd need to library the learnr package. Makes sense as it's in the learnr package.

@schloerke
Copy link
Collaborator Author

@wch Any idea on how to get around not having to require the user to say

safe(run_tutorial("slidly", package = "learnr", list(launch.browser = TRUE)))

as the run_tutorial is executed in a non interactive environment.

Environment variable?

@schloerke schloerke changed the title [WIP] safe expression evaluation with callr safe expression evaluation with callr Sep 6, 2018
@schloerke schloerke merged commit 90d45c6 into rstudio:master Sep 6, 2018
@schloerke schloerke deleted the scope branch September 6, 2018 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants