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
use_course() #196
use_course() #196
Conversation
These were already indirect dependencies anyway.
R/course.R
Outdated
httr::stop_for_status(dl$status_code) | ||
stopifnot( | ||
grepl("^https://dl.dropboxusercontent.com/content_link_zip/", dl$url) || | ||
grepl("^https://codeload.github.com", dl$url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure this should be very rigid for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are attaching httr, why are you not using it to download the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the answer to my question is to auto-find the filename.
I guess it feels like this code should really be in httr, not here, maybe as a extension on httr::write_disk()
.
If we do keep it here, because we don't plan on a httr release in the near future, I think it would be better to write a write_disk()
function and use it in a httr::GET()
request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this outside of httr, i.e. without access to unexported functions, doesn't look terribly practical. I think I can but it will definitely take me longer. How important is this @jimhester?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that important, but how important is it to have this auto naming in usethis in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very important for this workshop use case!
I know it's hard to believe, but getting a large group of people to download a specific set of files to a folder on their computer that
a) has a sane name
b) they can find again
c) includes ALL the files and no other files
is astonishingly difficult. And I am going to get them to do it, so help me God. Or die trying.
R/course.R
Outdated
hh <- curl::parse_headers_list(dl$headers) | ||
stopifnot(hh[["content-type"]] == "application/zip") | ||
content_disposition <- hh[["content-disposition"]] | ||
stopifnot(!is.null(content_disposition), nzchar(content_disposition)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too rigid? A filename could potentially also be generated from either the input url or dl$url
.
You can rename non-empty directories, I think the rename error is because the |
|
You could split the paths into individual directories, and iteratively peel off common prefixes. |
R/course.R
Outdated
## I know I could use regex and lookahead but this is easier for me to | ||
## maintain | ||
if (grepl("^\"", cd) && grepl("\"$", cd)) { | ||
cd <- gsub("^\"(.+)\"$", "\\1", cd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be sub()
not gsub()
, you are only doing at most one substitution per string, and the conditional is not needed, just use the substitution directly.
test <- function(cd) {
sub("^\"(.+)\"$", "\\1", cd)
}
test("foo/bar")
#> [1] "foo/bar"
test('"foo/bar"')
#> [1] "foo/bar"
test('foo/"bar"')
#> [1] "foo/\"bar\""
Created on 2018-01-09 by the reprex package (v0.1.1.9000).
R/course.R
Outdated
} | ||
message("content-disposition:\n", cd) | ||
|
||
cd <- gsub("^attachment;\\s*", "", cd, ignore.case = TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be sub()
it will only match (once) at the start of the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: it would be really nice to show progress from |
@jimhester Do you agree it's not possible / easy for me to get progress, given I am using |
I've made another major advance, so any comments are welcome. However, I'm not twiddling my thumbs. The usage example at the very top is has been updated for current state of things. |
I consider this ready for review now. It works on my Windows machine |
R/course.R
Outdated
#' Special-purpose function to download a folder of course materials. The only | ||
#' demand on the user is to confirm or specify where the new folder should be | ||
#' stored. Workflow: | ||
#' * User calls `use_course("SHORTLINK-GOES-HERE")`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shortlink = bit.ly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bit.ly more obvious now
R/course.R
Outdated
#' @param url Link to a ZIP file containing the materials, possibly behind a | ||
#' shortlink. Function developed with DropBox and GitHub in mind, but should | ||
#' work for ZIP files generally. See [download_zip()] for more. | ||
#' @param destdir The new folder is stored here. Defaults to working directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to default to the desktop? (Since everyone can find that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done d06440d. Usage examples at very top show this now.
R/course.R
Outdated
#' work for ZIP files generally. See [download_zip()] for more. | ||
#' @param destdir The new folder is stored here. Defaults to working directory. | ||
#' | ||
#' @return Path to the new directory holding the course materials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invisibly?
R/course.R
Outdated
#' ## demo with a small CRAN package available in various places | ||
#' | ||
#' ## from CRAN | ||
#' use_course("https://cran.r-project.org/bin/windows/contrib/3.4/rematch2_2.0.1.zip") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs example with shortlink too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reluctant to commit to this because I want the examples to work. But OK. I'll put a shortlink and make sure it works ... today.
#' ``` | ||
#' https://www.dropbox.com/sh/12345abcde/6789wxyz?dl=0 | ||
#' ``` | ||
#' Replace the `dl=0` at the end with `dl=1` to create a download link. The ZIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean purely mechanically, i.e. on the front-end?
If yes: I could special-case DropBox and check the input url and make this substitution.
Otherwise, I don't think so. If input is a shortlink, we have no visibility into the redirects, to get at this URL and fix it. The failure mode is also non-specific (Error: Download does not have MIME type 'application/zip'
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now learned this:
Information about any short bitly URL
http://bit.ly/x
is available athttp://bit.ly/x+
(that is, the URL with a plus sign appended), for examplehttp://bit.ly/1sNZMwL+
. This allows users to see and check the long URL before visiting it.
It's not designed for programmatic use but may be helpful. In any case, probably not doing this now.
R/course.R
Outdated
#' @return Path to the directory holding the unpacked files. | ||
#' @keywords internal | ||
#' @family download functions | ||
#' @export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, might be better to wait?
R/course.R
Outdated
keep <- function(file, | ||
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) { | ||
ignores <- paste0("(\\/|\\A)", gsub("\\.", "[.]", ignores), "(\\/|\\Z)") | ||
!any(vapply(ignores, function(x) grepl(x, file, perl = TRUE), logical(1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimhester is it worth having something like this in fs? Discarding paths matching globs is fairly common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably put something in there, we already do filtering in dir_ls(), so it wouldn't be too much work to wrap it in function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW just added fs::path_filter()
function r-lib/fs@195c8e4
R/course.R
Outdated
} | ||
|
||
keep <- function(file, | ||
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is my unzip-ignoring of .RData
going to be an ugly surprise for anyone? It's the only here that I wondered about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least, I've mentioned in the docs now.
R/course.R
Outdated
invisible(target) | ||
} | ||
|
||
keep <- function(file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keep()
could be rewritten somewhat more nicely.
In the following example keep_old
is the current implementation, keep_lgl()
is a tweaked form with the same return value. keep_re()
simply returns the files to be kept rather than a logical vector and keep()
uses fs::path_split()
and does not use any regular expression. They all have equivalent output.
keep_old <- function(file,
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) {
ignores <- paste0("(\\/|\\A)", gsub("\\.", "[.]", ignores), "(\\/|\\Z)")
!any(vapply(ignores, function(x) grepl(x, file, perl = TRUE), logical(1)))
}
keep_lgl <- function(file,
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) {
ignores <- paste0("((\\/|\\A)", gsub("\\.", "[.]", ignores), "(\\/|\\Z))", collapse = "|")
!grepl(ignores, file, perl = TRUE)
}
library(testthat)
files <- c("foo", "bar", ".Rproj.user", ".git", "/.git", "/.git/", ".git/",
"foo/.git", ".git", ".git/config", ".git/objects/06/3d3gysle", ".Rproj.user",
".Rproj.user/123jkl/persistent-state", ".Rhistory", ".RData", ".gitignore", "a/.gitignore", "foo.Rproj")
for (f in files) {
expect_identical(keep_lgl(!!f), keep_old(!!f))
}
keep_re <- function(file,
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) {
ignores <- paste0("((\\/|\\A)", gsub("\\.", "[.]", ignores), "(\\/|\\Z))", collapse = "|")
grep(ignores, file, perl = TRUE, value = TRUE, invert = TRUE)
}
keep_re(files)
#> [1] "foo" "bar" ".gitignore" "a/.gitignore"
#> [5] "foo.Rproj"
expect_equal(keep_re(files), files[vapply(files, keep_lgl, logical(1))])
keep <- function(file,
ignores = c(".Rproj.user", ".rproj.user", ".Rhistory", ".RData", ".git")) {
file[vapply(fs::path_split(file), function(p) !any(p %in% ignores), logical(1))]
}
keep(files)
#> [1] "foo" "bar" ".gitignore" "a/.gitignore"
#> [5] "foo.Rproj"
expect_equal(keep(files), keep_re(files))
Created on 2018-01-12 by the reprex package (v0.1.1.9000).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, adopted in 5275eff.
The logical version is more useful to me in development/inspection.
I think I've implemented all the feedback. Usage example updated to reflect new default to |
I think you should feel free to merge at this point; I'm sure we'll discover problems in the future, but this seems like a substantial amount of useful work. |
Starting to use this now. First public attempt next week. Can we follow the download with a call to |
Actually, just realized I can assign |
We made a conscious choice to NOT create and launch an RStudio Project with But I could be convinced. And/or we could make another function in the Thoughts, @hadley? We also learned at rstudio::conf that hitting a GitHub repo with Also, a couple people needed to update the curl and backports packages, so obviously I need to determine and enforce minimum versions of those dependencies. |
I built a repo for a course I'm teaching. The README has them install some packages, run |
Looks good! I'd love to get feedback on how it works for you. Just so you know, you can use a shortlink with |
Maybe we should automatically open a |
Used it last week and it was HUGE improvement over other ways I've tried to set up a class. Everyone had a project with the right structure so we no longer had path issues. |
This brings me actual joy |
I built a package to generate a repo skeleton. This way instructors can easily create a new repo with a few lines of code. Then the user can go and do This package creates the project on disk, populates the README with the desired packages and builds the file that students source to download data. Then it pushes all of that to GitHub so it is easily accessed by students. Please see what you think: https://github.com/jaredlander/RepoGenerator |
Fixes #132
use_course()
Goal:
usethis::use_course(SHORTLINK)
Usage with a GitHub ZIP behind a bit.ly shortlink. It's a package but you get the idea.
Usage with a DropBox ZIP from recent @hadley workshop. Slow because many big Keynote files. Realistic/worst case.