-
Notifications
You must be signed in to change notification settings - Fork 18
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
A few fixes for some issues #244
base: master
Are you sure you want to change the base?
Conversation
swapped interactive with ask (which was not used) which is v important to be able to leave code running without being prompted. Changed default file to second. The only year where two csvs are available is 2020 and for escooter data. Maybe this is interesting for some, but the other file should be the default. returning destfile works better for dependent functions get.R main change is to skip the read_ functions which involve multiple nested functions appear to be unneccesary and are a headache to debug. utils.R Main change is to fix find_file_name function which was looking for a csv with year name for years before 2018. This data is all in one file which has 1979 in.
Looking at this now Blaise... |
Hitting this issue with |
|
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.
Just a couple of comments for you @BlaiseKelly 🙏
#' Get data download dir | ||
#' @examples | ||
#' # get_data_directory() | ||
get_data_directory = 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.
Why remove this? It's needed in other functions.
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 I meant to move to get.R (as it took me a while to find where it was).
Have added to get.R
result = result[!grepl(pattern = "adjust", x = result)] | ||
result = result[grepl(pattern = years, x = result)] | ||
result = result[grepl(pattern = "1979", x = result)] |
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.
👍
R/get.R
Outdated
data_dir = data_dir, | ||
format = format) | ||
# read in from the file path defined above | ||
if(grepl(type, "vehicles", 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.
The first argument of grepl()
is pattern
so shouldn't "vehicles"
go before type
?
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.
Quite right! Have amended
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.
Looking good!
@@ -112,7 +112,7 @@ get_stats19 = function(year = NULL, | |||
) | |||
output_format = "tibble" | |||
} | |||
if (grepl(type, "casualties", ignore.case = TRUE) && output_format %in% c("sf", "ppp")) { | |||
if (grepl("casualties", type, ignore.case = TRUE) && output_format %in% c("sf", "ppp")) { |
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 (grepl("casualties", type, ignore.case = TRUE) && output_format %in% c("sf", "ppp")) { | |
if (grepl("cas", type, ignore.case = TRUE) && output_format %in% c("sf", "ppp")) { |
And save for "veh" and "acc|col" so more things match.
Thanks @BlaiseKelly almost there! Please can you take a look at
Source: https://github.com/ropensci/stats19/actions/runs/10091716542/job/27903832622?pr=244#step:6:260 Any ideas why? |
swapped interactive with ask (which was not used) which is v important to be able to leave code running without being prompted.
Changed default file to second. The only year where two csvs are available is 2020 and for escooter data. Maybe this is interesting for some, but the other file should be the default.
returning destfile works better for dependent functions
get.R
main change is to skip the read_ functions which involve multiple nested functions appear to be unneccesary and are a headache to debug.
utils.R
Main change is to fix find_file_name function which was looking for a csv with year name for years before 2018. This data is all in one file which has 1979 in.