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

Make data/ directory automatically accessible to exercises #539

Merged
merged 70 commits into from
Jul 9, 2021

Conversation

rossellhayes
Copy link
Contributor

This adds two interfaces for making files available to learnr tutorials.

  1. Any files in a data/ directory in the same directory as the tutorial Rmd document will be accessible to the tutorial.
  2. Calling the function use_remote_files() in the setup chunk will cache files into a temporary directory and make them available to the tutorial as if they were in the data/ directory.

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@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.

This is a great start! I really like how the interface is coming together and I think it will benefit many tutorial authors.

We'll need to think through and account for all of the various ways the underlying options can be set and that files can be added to an exercise. To help with that, I think the next step would be to add several small test tutorials in a folder at tests/testthat/exercise-files. Each one can demonstrate an expected use case. Actually writing the tests is going to be a little tricky, but we can work through that step together. In the mean time it will be helpful to have a few examples to test out interactively.

R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
R/file_helpers.R Outdated Show resolved Hide resolved
tests/testthat/test-exercise.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
R/initialize.R Outdated Show resolved Hide resolved
R/use_data_dir.R Outdated Show resolved Hide resolved
@gadenbuie
Copy link
Member

@rossellhayes I had a few small changes to propose and in the process I merged main into this branch. It turns out that our options-protection work misses options that are NULL. In other words withr::local_options() doesn't reset options that were NULL, so our tests are now failing until we fix that.

options(thing = NULL)
withr::with_options({
  options(thing = "bad")
})
getOption("thing")
#> "bad"

To be fair, this same issue will happen with the standard base R pattern too:

options(thing = NULL)
local({
  opts <- options()
  on.exit(options(opts), add = TRUE)
  options(thing = "bad")
})
getOption("thing")
#> "bad"

We'll have to write a custom options_restore() function that does handle NULL options.

@rossellhayes
Copy link
Contributor Author

rossellhayes commented Jun 30, 2021

It looks like this is a known issue: r-lib/withr#57
Will Landau proposes a solution here: r-lib/withr#57 (comment)

Edit: fixed in 6b32eeb

Merge branch 'data-files' of https://github.com/rossellhayes/learnr into data-files

# Conflicts:
#	sandbox/data_dir/file_test/file_test.Rmd
…rs, including `NULL` and `""`, to their original state on exit
…ions()` and envvars, including `NULL` and `""`, to their original state on exit
Copy link
Member

@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.

This looks great and I'm really excited for this feature to land in learnr. I'm approving the PR but we have a little bit more to do in terms of documentation.

  • Add an entry in NEWS under New Features
  • Document the new behavior in the learnr docs webpage. I think it makes sense to add a new section in docs/exercises.Rmd after Exercise Setup.
  • Once the above are done and the PR is merged, we'll need to follow up in any related issues to let everyone know about the new feature.

NEWS.md Outdated Show resolved Hide resolved
@gadenbuie gadenbuie requested a review from schloerke July 8, 2021 18:53
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.

LGTM given cosmetic suggestions!

R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
rossellhayes and others added 6 commits July 8, 2021 14:20
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
@gadenbuie gadenbuie merged commit 14d679a into rstudio:master Jul 9, 2021
@rossellhayes rossellhayes deleted the data-files branch July 20, 2021 21:40
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.

Helpers to handle local data files and file setup Data files on shiny server
4 participants