-
Notifications
You must be signed in to change notification settings - Fork 15
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
add read_absmaps()
caching
#64
Conversation
R/read_absmaps.R
Outdated
download.file(url, | ||
destfile = out_path) | ||
} else { | ||
if (stringr::str_detect(export_dir, "var.folders")) { |
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.
where does "var.folders"
come from?
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.
ahh some regex to check if the export_dir
is in a temporary location, but it won't work on Linux. I'll scrap it to be safe.
@@ -1,7 +1,8 @@ | |||
test_that("read_absmaps() works", { | |||
test_that("read_absmaps() retrieves objects", { | |||
|
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.
You should add testthat::skip_on_cran()
so that this test isn't executed on CRAN servers. Tests run on CRAN servers are executed in an environment without internet access, so your tests will fail
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'd also add testthat::skip_if_offline()
@@ -47,6 +50,10 @@ read_absmap <- function(name = NULL, | |||
warning("Both name and area/year entered. Defaulting to name value: ", name) | |||
} | |||
|
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.
(Genuine Q, not sure the answer) Do you want to add some kind of checking to see that the name (or area/year combination) is a valid one?
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 thought about that, but wanted to have a single point of truth (so that if there were items added to the absmapsdata
package, they could be accessed through strayr::read_absmap()
without having to update the strayr
package). And I didn't want to add an internet-dependent check in the case where the file is found locally.
But it's possible to meet those conditions -- I'll add it to this PR
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.
Yeah fair enough
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.
Looks good. I've left some specific comments, all minor.
If you're intending to work towards CRAN submission, you should be aware of this policy:
"Packages which use Internet resources should fail gracefully with an informative message if the resource is not available or has changed (and not give a check warning nor error)."
https://cran.r-project.org/web/packages/policies.html
To be honest I find this kind of head-scratchingly vague (what is 'graceful' about a failure)? The discussion here is useful: https://community.rstudio.com/t/internet-resources-should-fail-gracefully/49199. At a minimum I would make sure you skip_on_cran()
your test(s) that require internet access, as flagged in my comments
R/read_absmaps.R
Outdated
out_path <- file.path(export_dir, paste0(name, ".rda")) | ||
|
||
if (!file.exists(out_path)) { | ||
download.file(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.
Would it be worth wrapping this in tryCatch()
or purrr::safely()
or similar, so that you can at least provide an informative error if the download fails?
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.
have added graceful message with tryCatch()
.
R/read_absmaps.R
Outdated
|
||
if (!file.exists(out_path)) { | ||
download.file(url, | ||
destfile = out_path) |
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 would add mode = "wb"
. This specifies that the file is binary; this is automatically inferred on macOS and Linux, but as I understand it isn't always automatically inferred on Windows
@@ -1,7 +1,8 @@ | |||
test_that("read_absmaps() works", { | |||
test_that("read_absmaps() retrieves objects", { | |||
|
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'd also add testthat::skip_if_offline()
if (.validate_name) { | ||
tryCatch( | ||
download.file("https://github.com/wfmackey/absmapsdata/blob/master/data/absmapsdata_file_list.rda?raw=true", | ||
destfile = file.path(export_dir, "file_list.rda")), |
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 mode = "wb"
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 Hugh, added in the most recent commit: ae5bc41
add `read_absmaps()` caching
This adds a
export_dir
argument toread_absmaps()
which istempdir()
by default.re #63 and #62