Refactor/rearrange validation of arguments

ropensci · Sep 18, 2022 · ad4e304 · ad4e304
1 parent c644355
commit ad4e304
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 85 deletions.
diff --git a/R/search-pv.R b/R/search-pv.R
@@ -11,8 +11,7 @@ tojson_2 <- function(x, ...) {
 }
 
 #' @noRd
-to_arglist <- function(fields, subent_cnts, mtchd_subent_only,
-                       page, per_page, sort) {
+to_arglist <- function(fields, page, per_page, sort) {
   list(
     fields = fields,
     sort = list(as.list(sort)),
@@ -245,52 +244,18 @@ search_pv <- function(query,
                       api_key = Sys.getenv("PATENTSVIEW_API_KEY"),
                       ...) {
 
-  if (identical(api_key, "")) {
-    stop2("Need API key")
-  }
-  if (!is.null(error_browser)) {
-    lifecycle::deprecate_warn(when = "0.2.0", what = "search_pv(error_browser)")
-  }
-  # Was previously defaulting to FALSE and we're still defaulting to FALSE to
-  # mirror the fact that the API doesn't support subent_cnts. Warn only if user
-  # tries to set subent_cnts to TRUE.
-  if (isTRUE(subent_cnts)) {
-    lifecycle::deprecate_warn(
-      when = "1.0.0",
-      what = "search_pv(subent_cnts)",
-      details = "The new version of the API does not support subentity counts."
-    )
-  }
-  # Was previously defaulting to TRUE and now we're defaulting to FALSE, hence
-  # we're being more chatty here than with subent_cnts.
-  if (lifecycle::is_present(mtchd_subent_only)) {
-    lifecycle::deprecate_warn(
-      when = "1.0.0",
-      what = "search_pv(mtchd_subent_only)",
-      details = "Non-matched subentities will always be returned under the new
-      version of the API"
-    )
-  }
-
-  validate_endpoint(endpoint)
+  validate_args(api_key, fields, endpoint, method, page, per_page, sort)
+  deprecate_warn_all(error_browser, subent_cnts, mtchd_subent_only)
 
   if (is.list(query)) {
-    # check_query(query, endpoint)
+    check_query(query, endpoint)
     query <- jsonlite::toJSON(query, auto_unbox = TRUE)
   }
 
-  # validate_misc_args(
-  #   query, fields, endpoint, method, subent_cnts, mtchd_subent_only, page,
-  #   per_page, sort
-  # )
-
-  arg_list <- to_arglist(
-    fields, subent_cnts, mtchd_subent_only, page, per_page, sort
-  )
-
+  arg_list <- to_arglist(fields, page, per_page, sort)
   base_url <- get_base(endpoint)
-
   res <- one_request(method, query, base_url, arg_list, api_key, ...)
+
   # TODO(cbaker): better naming here
   res <- process_resp(res)
 

diff --git a/R/utils.R b/R/utils.R
@@ -12,33 +12,6 @@ format_num <- function(x) {
   )
 }
 
-#' @noRd
-validate_endpoint <- function(endpoint) {
-  ok_ends <- get_endpoints()
-
-  asrt(
-    all(endpoint %in% ok_ends, length(endpoint) == 1),
-    "endpoint must be one of the following: ", paste(ok_ends, collapse = ", ")
-  )
-}
-
-#' @noRd
-validate_groups <- function(groups) {
-  ok_grps <- unique(fieldsdf$group)
-  asrt(
-    all(groups %in% ok_grps),
-    "group must be one of the following: ", paste(ok_grps, collapse = ", ")
-  )
-}
-
-#' @noRd
-validate_pv_data <- function(data) {
-  asrt(
-    "pv_data_result" %in% class(data),
-    "Wrong input type for data...See example for correct input type"
-  )
-}
-
 #' @noRd
 to_singular <- function(plural) {
   if (endsWith(plural, "ees")) {
@@ -54,7 +27,6 @@ to_singular <- function(plural) {
   }
 }
 
-
 #' @noRd
 to_plural <- function(singular) {
   if (endsWith(singular, "y")) {

diff --git a/R/validate-args.R b/R/validate-args.R
@@ -1,31 +1,40 @@
 #' @noRd
-validate_misc_args <- function(query, fields, endpoint, method, subent_cnts,
-                               mtchd_subent_only, page, per_page, sort) {
-
-  ok_meth <- c("GET", "POST")
+validate_endpoint <- function(endpoint) {
+  ok_ends <- get_endpoints()
   asrt(
-    all(method %in% ok_meth, length(method) == 1),
-    "method must be either 'GET' or 'POST'"
+    all(endpoint %in% ok_ends, length(endpoint) == 1),
+    "endpoint must be one of the following: ", paste(ok_ends, collapse = ", ")
   )
+}
 
+#' @noRd
+validate_args <- function(api_key, fields, endpoint, method, page, per_page,
+                          sort) {
   asrt(
-    all(is.logical(subent_cnts), length(subent_cnts) == 1),
-    "subent_cnts must be either TRUE or FALSE"
+    !identical(api_key, ""),
+    "The new version of the API requires an API key"
   )
+
+  flds_flt <- fieldsdf[fieldsdf$endpoint == endpoint, "field"]
   asrt(
-    all(is.logical(mtchd_subent_only), length(mtchd_subent_only) == 1),
-    "mtchd_subent_only must be either TRUE or FALSE"
+    all(fields %in% flds_flt),
+    "Bad field(s): ", paste(fields[!(fields %in% flds_flt)], collapse = ", ")
   )
 
+  validate_endpoint(endpoint)
+
+  asrt(
+    all(method %in% c("GET", "POST"), length(method) == 1),
+    "method must be either 'GET' or 'POST'"
+  )
   asrt(
     all(is.numeric(page), length(page) == 1, page >= 1),
     "page must be a numeric value greater than 1"
   )
   asrt(
-    all(is.numeric(per_page), length(per_page) == 1, per_page <= 10000),
-    "per_page must be a numeric value less than or equal to 10,000"
+    all(is.numeric(per_page), length(per_page) == 1, per_page <= 1000),
+    "per_page must be a numeric value less than or equal to 1,000"
   )
-
   if (!is.null(sort))
     asrt(
       all(
@@ -34,10 +43,48 @@ validate_misc_args <- function(query, fields, endpoint, method, subent_cnts,
       "sort has to be a named character vector and each name has to be ",
       "specified in the field argument. See examples"
     )
+}
 
-  flds_flt <- fieldsdf[fieldsdf$endpoint == endpoint, "field"]
+#' @noRd
+validate_groups <- function(groups) {
+  ok_grps <- unique(fieldsdf$group)
   asrt(
-    all(fields %in% flds_flt),
-    "Bad field(s): ", paste(fields[!(fields %in% flds_flt)], collapse = ", ")
+    all(groups %in% ok_grps),
+    "group must be one of the following: ", paste(ok_grps, collapse = ", ")
   )
 }
+
+#' @noRd
+validate_pv_data <- function(data) {
+  asrt(
+    "pv_data_result" %in% class(data),
+    "Wrong input type for data...See example for correct input type"
+  )
+}
+
+#' @noRd
+deprecate_warn_all <- function(error_browser, subent_cnts, mtchd_subent_only) {
+  if (!is.null(error_browser)) {
+    lifecycle::deprecate_warn(when = "0.2.0", what = "search_pv(error_browser)")
+  }
+  # Was previously defaulting to FALSE and we're still defaulting to FALSE to
+  # mirror the fact that the API doesn't support subent_cnts. Warn only if user
+  # tries to set subent_cnts to TRUE.
+  if (isTRUE(subent_cnts)) {
+    lifecycle::deprecate_warn(
+      when = "1.0.0",
+      what = "search_pv(subent_cnts)",
+      details = "The new version of the API does not support subentity counts."
+    )
+  }
+  # Was previously defaulting to TRUE and now we're defaulting to FALSE, hence
+  # we're being more chatty here than with subent_cnts.
+  if (lifecycle::is_present(mtchd_subent_only)) {
+    lifecycle::deprecate_warn(
+      when = "1.0.0",
+      what = "search_pv(mtchd_subent_only)",
+      details = "Non-matched subentities will always be returned under the new
+      version of the API"
+    )
+  }
+}