Merge pull request #13 from PIP-Technical-Team/code_review

Code review

Author: Aeilert

.Rbuildignore

@@ -6,4 +6,5 @@
 ^_pkgdown\.yml$
 ^docs$
 ^pkgdown$
-TEMP/
+TEMP/
+^data-raw$

R/get_aux.R

@@ -17,7 +17,7 @@
 #' # Get countries
 #' df <- get_aux("countries")
 get_aux <- function(table = NULL, version = NULL, api_version = "v1",
-                    format = c("json", "csv", "rds"),
+                    format = c("rds", "json", "csv"),
                     simplify = TRUE, server = NULL) {
 
   # Match args
@@ -64,7 +64,7 @@ get_aux <- function(table = NULL, version = NULL, api_version = "v1",
 #' # Short hand to get countries
 #' get_countries()
 get_countries <- function(version = NULL, api_version = "v1",
-                          format = c("json", "csv", "rds"),
+                          format = c("rds", "json", "csv"),
                           server = NULL) {
   get_aux("countries",
           version = version, api_version = api_version,
@@ -80,7 +80,7 @@ get_countries <- function(version = NULL, api_version = "v1",
 #' # Short hand to get regions
 #' get_regions()
 get_regions <- function(version = NULL, api_version = "v1",
-                        format = c("json", "csv", "rds"),
+                        format = c("rds", "json", "csv"),
                         server = NULL) {
   get_aux("regions",
           version = version, api_version = api_version,

R/get_stats.R

@@ -58,10 +58,10 @@ get_stats <- function(country = "all",
                       reporting_level = c("all", "national", "urban", "rural"),
                       version = NULL,
                       api_version = "v1",
-                      format = c("json", "csv", "rds"),
+                      format = c("rds", "json", "csv"),
                       simplify = TRUE,
                       server = NULL) {
-
+  # browser()
   # Match args
   welfare_type <- match.arg(welfare_type)
   reporting_level <- match.arg(reporting_level)

R/sysdata.rda

@@ -1,5 +1,3 @@
-base_url <- "https://api.worldbank.org/pip"
-
 #' check_internet
 #' @noRd
 check_internet <- function() {
@@ -40,21 +38,13 @@ check_status <- function(res, parsed) {
 }
 
 #' build_url
-#' @param server Server
-#' @param endpoint Endpoint
+#' @param server character: Server
+#' @param endpoint character: Endpoint
+#' @param api_version character: API version
 #' @inheritParams get_stats
 #' @noRd
 build_url <- function(server, endpoint, api_version) {
-  if (!is.null(server)) {
-    match.arg(server, c("prod", "qa", "dev"))
-    if (server == "qa") base_url <- Sys.getenv("PIP_QA_URL")
-    if (server == "dev") base_url <- Sys.getenv("PIP_DEV_URL")
-    attempt::stop_if(
-      base_url == "",
-      msg = sprintf("'%s' url not found. Check your .Renviron file.", server)
-    )
-  }
-  if (is.null(server) || server == "prod") base_url <- base_url
+  base_url <- select_base_url(server = server)
   sprintf("%s/%s/%s", base_url, api_version, endpoint)
 }
 
@@ -131,3 +121,30 @@ parse_response <- function(res, simplify) {
     )
   }
 }
+
+#' Select base URL
+#'
+#' Helper function to switch base URLs depending on PIP server being used
+#'
+#' @param server character: c("prod", "qa", "dev"). Defaults to NULL (ie. prod).
+#' @return character
+#' @noRd
+select_base_url <- function(server) {
+  if (!is.null(server)) {
+    match.arg(server, c("prod", "qa", "dev"))
+    if (server %in% c("qa", "dev")) {
+      if (server == "qa") base_url <- Sys.getenv("PIP_QA_URL")
+      if (server == "dev") base_url <- Sys.getenv("PIP_DEV_URL")
+      attempt::stop_if(
+        base_url == "",
+        msg = sprintf("'%s' url not found. Check your .Renviron file.", server)
+      )
+    }
+  }
+
+  if (is.null(server) || server == "prod") {
+    base_url <- prod_url
+  }
+
+  return(base_url)
+}

R/utils.R

@@ -3,6 +3,9 @@
     cm <- cachem::cache_mem(max_size = 512 * 1024^2, evict = "lru")
     get_stats <<- memoise::memoise(get_stats, cache = cm)
     get_aux <<- memoise::memoise(get_aux, cache = cm)
-    packageStartupMessage("Info: Session based caching is enabled.")
   }
 }
+
+.onAttach <- function(libname, pkgname) {
+  packageStartupMessage("Info: Session based caching is enabled.")
+}

R/zzz.R

@@ -0,0 +1,6 @@
+## code to prepare `data` dataset goes here
+prod_url <- "https://api.worldbank.org/pip"
+
+usethis::use_data(prod_url,
+                  internal = TRUE,
+                  overwrite = TRUE)

data-raw/data.R

@@ -31,11 +31,11 @@ test_that("build_url() works", {
 
   # Check that url is correctly pasted together
   x <- build_url(server = NULL, endpoint = "pip", api_version = "v1")
-  expect_identical(x, paste0(base_url, "/v1/pip"))
+  expect_identical(x, paste0(prod_url, "/v1/pip"))
   x <- build_url("prod", "pip", api_version = "v1")
-  expect_identical(x, paste0(base_url, "/v1/pip"))
+  expect_identical(x, paste0(prod_url, "/v1/pip"))
   x <- build_url("prod", "pip-grp", api_version = "v2")
-  expect_identical(x, paste0(base_url, "/v2/pip-grp"))
+  expect_identical(x, paste0(prod_url, "/v2/pip-grp"))
 
   # Expect error if server arg is incorrect
   expect_error(build_url("tmp", "pip", "v1"))