Version 1.0.0 #64
Version 1.0.0 #64
Conversation
…documentation on multiple other functions.
There was a problem hiding this comment.
It looks like get_unit_code, get_park_code, get_unit_code_info, get_unit_info share a lot of code in common (including the API request I think?). At some point, not necessarily as part of this PR, it might be good to have a single function that fetches the unit info from IRMA and gets called by the rest.
In general looks good, and I don't think anything I've commented on necessarily has to change for this release.
| @@ -11,7 +11,7 @@ | |||
| #' | |||
| #' @importFrom magrittr %>% | |||
There was a problem hiding this comment.
Instead of importing the magrittr pipe in individual functions, I'd recommend using usethis::use_pipe() to make it available throughout the whole package (and, optionally, re-export it so that users don't have to load magrittr in order to use the pipe themselves - not sure what the best practice on that is though)
There was a problem hiding this comment.
after some googling, I can't find any reason not to re-export the pipe (usethis::use_pipe(export = TRUE))
| alpha <- dat %>% dplyr::filter(grepl(unit, FullName, ignore.case = TRUE)) # filter FullName for input | ||
| dat <- XML::xmlToDataFrame(result) | ||
| # pare down the output some | ||
| dat <- dat[, c(1, 3, 5, 7, 8, 9, 11, 13)] |
There was a problem hiding this comment.
Maybe consider referring to columns by name here, for clarity and so that we don't get weird behavior if column order changes
There was a problem hiding this comment.
similar situation in the rest of the functions here
|
These are definitely the oldest functions in the package and need a re-write to consolidate them, reduce API calls and file downloads, and probably even some thinking about whether they should be included in the package at all (what is their utility?). But for now, it works so merging ahead! |
Makes sense! I could see these functions, or an updated version of them, eventually becoming part of an R package that's just a wrapper for the DataStore API (but I think that involves some work on the API itself first). |
Includes breaking changes such as removing
get_data_package_deprecatedand changes to documentation in multiple functions.