From 4e5ae63f5ed4c6fc5b8ad336a9dc0fc9ab73b7f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Thu, 2 Feb 2023 15:53:23 +0100 Subject: [PATCH 01/20] refactor: replace {crul} with {httr2} --- DESCRIPTION | 2 +- NEWS.md | 2 ++ R/oc_api_ok.R | 7 ++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index aeeeeaba..f9b035ff 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -25,8 +25,8 @@ BugReports: https://github.com/ropensci/opencage/issues Depends: R (>= 3.4.0) Imports: - crul (>= 0.5.2), dplyr (>= 0.7.4), + httr2, jsonlite (>= 1.5), lifecycle, memoise (>= 1.1.0), diff --git a/NEWS.md b/NEWS.md index a1df7afe..d939e830 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # opencage (development version) + +* http requests are now handled by {[httr2](https://httr2.r-lib.org/)}, not {[crul](https://docs.ropensci.org/crul/)} ([#156](https://github.com/ropensci/opencage/issues/156)). * opencage now supports an `address_only` parameter, see "[New optional API parameter 'address_only'](https://blog.opencagedata.com/post/new-optional-parameter-addressonly)", ([#151](https://github.com/ropensci/opencage/pull/151)). * The geocoding functions will not send a query to the API anymore if no API key is present ([#133](https://github.com/ropensci/opencage/issues/133)). * `NA`s are allowed again for the `placename` or `latitude`/`longitude` arguments (also empty strings for `placename`). diff --git a/R/oc_api_ok.R b/R/oc_api_ok.R index ec1cb52a..fc8b79cc 100644 --- a/R/oc_api_ok.R +++ b/R/oc_api_ok.R @@ -10,5 +10,10 @@ #' @keywords internal oc_api_ok <- function(url = "https://api.opencagedata.com") { - crul::ok(url, useragent = "https://github.com/ropensci/opencage") + resp <- httr2::request("https://api.opencagedata.com") %>% + httr2::req_method("HEAD") %>% + httr2::req_user_agent("https://github.com/ropensci/opencage") %>% + httr2::req_perform() + + !httr2::resp_is_error(resp) } From 2fd20d8ed8aa80fc722082d4432d65ffd62e06dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Thu, 2 Feb 2023 16:04:18 +0100 Subject: [PATCH 02/20] less crul --- R/oc_api_ok.R | 2 +- R/oc_process.R | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/R/oc_api_ok.R b/R/oc_api_ok.R index fc8b79cc..7dc3b7de 100644 --- a/R/oc_api_ok.R +++ b/R/oc_api_ok.R @@ -12,7 +12,7 @@ oc_api_ok <- function(url = "https://api.opencagedata.com") { resp <- httr2::request("https://api.opencagedata.com") %>% httr2::req_method("HEAD") %>% - httr2::req_user_agent("https://github.com/ropensci/opencage") %>% + httr2::req_user_agent(oc_ua_string) %>% httr2::req_perform() !httr2::resp_is_error(resp) diff --git a/R/oc_process.R b/R/oc_process.R index d52c74c9..8273456e 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -236,8 +236,7 @@ oc_build_url <- function(query_par, endpoint) { oc_path <- paste0("geocode/v1/", endpoint) - crul::url_build( - url = "https://api.opencagedata.com", + list( path = oc_path, query = query_par ) @@ -246,18 +245,19 @@ oc_build_url <- function(query_par, endpoint) { #' GET request from OpenCage #' -#' @param oc_url character string URL with query parameters, built with +#' @param oc_url_parts list with path and query, built with #' `oc_build_url()` #' -#' @return crul::HttpResponse object +#' @return httr2 response #' @noRd -oc_get <- function(oc_url) { - client <- crul::HttpClient$new( - url = oc_url, - headers = list(`User-Agent` = oc_ua_string) - ) - client$get() +oc_get <- function(oc_url_parts) { + + httr2::request("https://api.opencagedata.com") %>% + httr2::req_url_path(oc_url[["path"]]) %>% + httr2::req_url_query(oc_url[["query"]]) %>% + httr2::req_user_agent(oc_ua_string) %>% + httr2::req_perform() } # user-agent string: this is set at build-time, but that should be okay, From d7e17d02fe9730e67d93c785d7f81d6ced8be8d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 08:29:13 +0100 Subject: [PATCH 03/20] some progress --- DESCRIPTION | 2 +- NAMESPACE | 1 + NEWS.md | 2 +- R/oc_config.R | 7 ------ R/oc_process.R | 42 ++++++++++----------------------- R/zzz.R | 22 +++++------------ man/oc_config.Rd | 9 ++++--- tests/testthat/test-oc_config.R | 19 --------------- tests/testthat/test-oc_get.R | 11 --------- 9 files changed, 25 insertions(+), 90 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index f9b035ff..bbaad734 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -29,10 +29,10 @@ Imports: httr2, jsonlite (>= 1.5), lifecycle, + magrittr, memoise (>= 1.1.0), progress (>= 1.1.2), purrr (>= 0.2.4), - ratelimitr (>= 0.4.0), rlang, tibble (>= 1.4.2), tidyr (>= 0.8.0), diff --git a/NAMESPACE b/NAMESPACE index 8c03a7d5..534673b3 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -26,4 +26,5 @@ export(oc_reverse_df) export(opencage_forward) export(opencage_key) export(opencage_reverse) +importFrom(magrittr,`%>%`) importFrom(rlang,.data) diff --git a/NEWS.md b/NEWS.md index d939e830..79cc3a4a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,7 @@ # opencage (development version) -* http requests are now handled by {[httr2](https://httr2.r-lib.org/)}, not {[crul](https://docs.ropensci.org/crul/)} ([#156](https://github.com/ropensci/opencage/issues/156)). +* http requests are now handled by {[httr2](https://httr2.r-lib.org/)}, not {[crul](https://docs.ropensci.org/crul/)}; and rate limitation / throttling by httr2, not [ratelimitr](https://github.com/tarakc02/ratelimitr) ([#156](https://github.com/ropensci/opencage/issues/156)). * opencage now supports an `address_only` parameter, see "[New optional API parameter 'address_only'](https://blog.opencagedata.com/post/new-optional-parameter-addressonly)", ([#151](https://github.com/ropensci/opencage/pull/151)). * The geocoding functions will not send a query to the API anymore if no API key is present ([#133](https://github.com/ropensci/opencage/issues/133)). * `NA`s are allowed again for the `placename` or `latitude`/`longitude` arguments (also empty strings for `placename`). diff --git a/R/oc_config.R b/R/oc_config.R index 499557da..08b3fc16 100644 --- a/R/oc_config.R +++ b/R/oc_config.R @@ -91,7 +91,6 @@ #' @export oc_config <- function(key = Sys.getenv("OPENCAGE_KEY"), - rate_sec = getOption("oc_rate_sec", default = 1L), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), ...) { @@ -119,12 +118,6 @@ oc_config <- Sys.setenv(OPENCAGE_KEY = pat) - # set rate limit - ratelimitr::UPDATE_RATE( - oc_get_limited, - ratelimitr::rate(n = rate_sec, period = 1L) - ) - # set no_record oc_check_logical(no_record, check_length_one = TRUE) options("oc_no_record" = no_record) diff --git a/R/oc_process.R b/R/oc_process.R index 8273456e..0259f552 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -171,8 +171,6 @@ oc_process <- # parse response res_text <- oc_parse_text(res_env) - # check status message - oc_check_status(res_env, res_text) } else { # Fake 0 results response @@ -253,11 +251,18 @@ oc_build_url <- function(query_par, endpoint) { oc_get <- function(oc_url_parts) { - httr2::request("https://api.opencagedata.com") %>% - httr2::req_url_path(oc_url[["path"]]) %>% - httr2::req_url_query(oc_url[["query"]]) %>% + req_with_url <- httr2::request("https://api.opencagedata.com") %>% + httr2::req_url_path_append(oc_url_parts[["path"]]) + + # some gymnastics needed as we can't pass the query as a list to httr2? + args <- oc_url_parts[["query"]] + args[[".req"]] <- req_with_url + + do.call(what = httr2::req_url_query, args = args) %>% + httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% httr2::req_user_agent(oc_ua_string) %>% - httr2::req_perform() + httr2::req_perform() %>% + httr2::resp_check_status() } # user-agent string: this is set at build-time, but that should be okay, @@ -277,36 +282,13 @@ oc_ua_string <- #' @noRd oc_parse_text <- function(res) { - text <- res$parse(encoding = "UTF-8") + text <- httr2::resp_body_string(res) if (identical(text, "")) { stop("OpenCage returned an empty response.", call. = FALSE) } text } - -#' Check the status of the HttpResponse -#' -#' @param res_env crul::HttpResponse object -#' @param res_text parsed HttpResponse -#' -#' @return NULL if status code less than or equal to 201, else `stop()` -#' @noRd - -oc_check_status <- function(res_env, res_text) { - if (res_env$success()) { - return(invisible()) - } - message <- - jsonlite::fromJSON( - res_text, - simplifyVector = TRUE, - flatten = TRUE - )$status$message - stop("HTTP failure: ", res_env$status_code, "\n", message, call. = FALSE) -} - - #' Format the result string #' #' @param res_text parsed HttpResponse diff --git a/R/zzz.R b/R/zzz.R index 26c18d1a..19b3a076 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -1,27 +1,17 @@ +#' @importFrom magrittr `%>%` + # We use `<<-` below to modify the package's namespace. # It doesn't modify the global environment. -# We do this to prevent build time dependencies on {memoise} and {ratelimitr}, +# We do this to prevent build time dependencies on {memoise}, # as recommended in . # Cf. for further details. -# First make sure that the functions are defined at build time -oc_get_limited <- oc_get -oc_get_memoise <- oc_get_limited +# First make sure that the function is defined at build time +oc_get_memoise <- oc_get # Then modify them at load-time # nocov start .onLoad <- function(libname, pkgname) { - # limit requests per second - oc_get_limited <<- - ratelimitr::limit_rate( - oc_get, - # rate can be changed via oc_config()/ratelimitr::UPDATE_RATE() - ratelimitr::rate( - n = 1L, - period = 1L - ) - ) - - oc_get_memoise <<- memoise::memoise(oc_get_limited) + oc_get_memoise <<- memoise::memoise(oc_get) } # nocov end diff --git a/man/oc_config.Rd b/man/oc_config.Rd index c20e23bd..e79c84c4 100644 --- a/man/oc_config.Rd +++ b/man/oc_config.Rd @@ -6,7 +6,6 @@ \usage{ oc_config( key = Sys.getenv("OPENCAGE_KEY"), - rate_sec = getOption("oc_rate_sec", default = 1L), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), ... @@ -16,10 +15,6 @@ oc_config( \item{key}{Your OpenCage API key as a character vector of length one. Do not pass the key directly as a parameter, though. See details.} -\item{rate_sec}{Numeric vector of length one. Sets the maximum number of -requests sent to the OpenCage API per second. Defaults to the value set in -the \code{oc_rate_sec} option, or, in case that does not exist, to 1L.} - \item{no_record}{Logical vector of length one. When \code{TRUE}, OpenCage will not create log entries of the queries and will not cache the geocoding requests. Defaults to the value set in the \code{oc_no_record} option, or, in @@ -33,6 +28,10 @@ not \code{TRUE}, the API key will be replaced with the string \code{OPENCAGE_KEY case that does not exist, to \code{FALSE}.} \item{...}{Ignored.} + +\item{rate_sec}{Numeric vector of length one. Sets the maximum number of +requests sent to the OpenCage API per second. Defaults to the value set in +the \code{oc_rate_sec} option, or, in case that does not exist, to 1L.} } \description{ Configure session settings for \pkg{opencage}. diff --git a/tests/testthat/test-oc_config.R b/tests/testthat/test-oc_config.R index a23de06f..7f44c932 100644 --- a/tests/testthat/test-oc_config.R +++ b/tests/testthat/test-oc_config.R @@ -48,25 +48,6 @@ test_that("oc_config throws error with faulty OpenCage key", { ) }) -# test rate_sec argument -------------------------------------------------- - -test_that("oc_config updates rate limit of oc_get_limit", { - # make sure there is a key present - withr::local_envvar(c("OPENCAGE_KEY" = key_200)) - - rps <- 5L - oc_config(rate_sec = rps) - expect_identical(ratelimitr::get_rates(oc_get_limited)[[1]][["n"]], rps) - - rps <- 3L - oc_config(rate_sec = rps) - expect_identical(ratelimitr::get_rates(oc_get_limited)[[1]][["n"]], rps) - - # set rate_sec back to default: 1L/sec - oc_config() - expect_identical(ratelimitr::get_rates(oc_get_limited)[[1]][["n"]], 1L) -}) - # test no_record argument ------------------------------------------------- test_that("oc_config sets no_record option", { diff --git a/tests/testthat/test-oc_get.R b/tests/testthat/test-oc_get.R index b55f3515..f88cd732 100644 --- a/tests/testthat/test-oc_get.R +++ b/tests/testthat/test-oc_get.R @@ -56,17 +56,6 @@ test_that("oc_get returns a response object for vector countrycode", { ) }) -test_that("oc_get_limited is rate limited", { - skip_on_cran() - skip_if_offline("httpbin.org") - - tm <- system.time({ - replicate(2, oc_get_limited("https://httpbin.org/get")) - }) - rate <- ratelimitr::get_rates(oc_get_limited) - expect_gte(tm[["elapsed"]], rate[[1]][["period"]] / rate[[1]][["n"]]) -}) - test_that("oc_get_memoise memoises", { skip_on_cran() skip_if_offline("httpbin.org") From fd93371fce53a4165cafa692ce9c5e691c032993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 08:35:28 +0100 Subject: [PATCH 04/20] fixes --- R/oc_api_ok.R | 3 ++- R/oc_process.R | 3 +-- tests/testthat/test-oc_build_url.R | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/R/oc_api_ok.R b/R/oc_api_ok.R index 7dc3b7de..abb41c7e 100644 --- a/R/oc_api_ok.R +++ b/R/oc_api_ok.R @@ -10,9 +10,10 @@ #' @keywords internal oc_api_ok <- function(url = "https://api.opencagedata.com") { - resp <- httr2::request("https://api.opencagedata.com") %>% + resp <- httr2::request(url) %>% httr2::req_method("HEAD") %>% httr2::req_user_agent(oc_ua_string) %>% + httr2::req_error(is_error = function(resp) FALSE) %>% httr2::req_perform() !httr2::resp_is_error(resp) diff --git a/R/oc_process.R b/R/oc_process.R index 0259f552..e1e592fe 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -261,8 +261,7 @@ oc_get <- function(oc_url_parts) { do.call(what = httr2::req_url_query, args = args) %>% httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% httr2::req_user_agent(oc_ua_string) %>% - httr2::req_perform() %>% - httr2::resp_check_status() + httr2::req_perform() # will error if API error :-) } # user-agent string: this is set at build-time, but that should be okay, diff --git a/tests/testthat/test-oc_build_url.R b/tests/testthat/test-oc_build_url.R index f5279875..7cc75dc8 100644 --- a/tests/testthat/test-oc_build_url.R +++ b/tests/testthat/test-oc_build_url.R @@ -1,9 +1,9 @@ -test_that("oc_build_url returns a string", { +test_that("oc_build_url returns a list", { expect_type( oc_build_url( query_par = list(placename = "Haarlem"), endpoint = "json" ), - "character" + "list" ) }) From 10577f90968eb68df288c657e96aaab059a95771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 08:44:26 +0100 Subject: [PATCH 05/20] refactor --- R/oc_process.R | 26 +++++++++++++++++++------- tests/testthat/test-oc_clear_cache.R | 6 +++--- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/R/oc_process.R b/R/oc_process.R index e1e592fe..18792108 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -249,16 +249,28 @@ oc_build_url <- function(query_par, endpoint) { #' @return httr2 response #' @noRd -oc_get <- function(oc_url_parts) { +oc_get <- function(oc_url_parts = NULL) { - req_with_url <- httr2::request("https://api.opencagedata.com") %>% - httr2::req_url_path_append(oc_url_parts[["path"]]) + initial_req <- httr2::request("https://api.opencagedata.com") - # some gymnastics needed as we can't pass the query as a list to httr2? - args <- oc_url_parts[["query"]] - args[[".req"]] <- req_with_url + req_with_url <- if (!is.null(oc_url_parts[["path"]])) { + httr2::req_url_path_append(initial_req, oc_url_parts[["path"]]) + } else { + initial_req + } + + + if (!is.null(oc_url_parts[["query"]])) { + # some gymnastics needed as we can't pass the query as a list to httr2? + args <- oc_url_parts[["query"]] + args[[".req"]] <- req_with_url + + query_req <- do.call(what = httr2::req_url_query, args = args) + } else { + query_req <- req_with_url + } - do.call(what = httr2::req_url_query, args = args) %>% + query_req %>% httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% httr2::req_user_agent(oc_ua_string) %>% httr2::req_perform() # will error if API error :-) diff --git a/tests/testthat/test-oc_clear_cache.R b/tests/testthat/test-oc_clear_cache.R index 64581e12..e1131618 100644 --- a/tests/testthat/test-oc_clear_cache.R +++ b/tests/testthat/test-oc_clear_cache.R @@ -5,8 +5,8 @@ test_that("oc_clear_cache clears cache", { # until a memoise >v.1.1 is released, we need to run oc_get_memoise() twice to # have it really cache results # https://github.com/ropensci/opencage/pull/87#issuecomment-573573183 - replicate(2, oc_get_memoise("https://httpbin.org/get")) - expect_true(memoise::has_cache(oc_get_memoise)("https://httpbin.org/get")) + replicate(2, oc_get_memoise()) + expect_true(memoise::has_cache(oc_get_memoise)()) oc_clear_cache() - expect_false(memoise::has_cache(oc_get_memoise)("https://httpbin.org/get")) + expect_false(memoise::has_cache(oc_get_memoise)()) }) From b2570066dd86e3276771d061e48c38b1c1eecd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 08:53:17 +0100 Subject: [PATCH 06/20] fix --- R/oc_process.R | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/R/oc_process.R b/R/oc_process.R index 18792108..56642881 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -130,7 +130,7 @@ oc_process <- } # build url - oc_url <- oc_build_url( + oc_url_parts <- oc_build_url( query_par = list( q = query, bounds = bounds, @@ -150,6 +150,8 @@ oc_process <- ), endpoint = endpoint ) + query_req <- build_query_with_req(oc_url_parts) + oc_url <- query_req[["url"]] # return url only if (return == "url_only") { @@ -251,6 +253,15 @@ oc_build_url <- function(query_par, endpoint) { oc_get <- function(oc_url_parts = NULL) { + query_req <- build_query_with_req(oc_url_parts) + + query_req %>% + httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% + httr2::req_user_agent(oc_ua_string) %>% + httr2::req_perform() # will error if API error :-) +} + +build_query_with_req <- function(oc_url_parts) { initial_req <- httr2::request("https://api.opencagedata.com") req_with_url <- if (!is.null(oc_url_parts[["path"]])) { @@ -270,10 +281,7 @@ oc_get <- function(oc_url_parts = NULL) { query_req <- req_with_url } - query_req %>% - httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% - httr2::req_user_agent(oc_ua_string) %>% - httr2::req_perform() # will error if API error :-) + query_req } # user-agent string: this is set at build-time, but that should be okay, From 06565c84aa1d1508e2c3cd8038c271d9d47dda90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 08:55:46 +0100 Subject: [PATCH 07/20] ouch --- R/oc_process.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/oc_process.R b/R/oc_process.R index 56642881..bff8b1b7 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -168,7 +168,7 @@ oc_process <- # send query to API if not NA, else return (fake) empty res_text if (!is.na(query) && nchar(query) >= 2) { # get response - res_env <- oc_get_memoise(oc_url) + res_env <- oc_get_memoise(oc_url_parts) # parse response res_text <- oc_parse_text(res_env) From 023971a9b203107ec4f50db51d2f4a425d281fc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:06:54 +0100 Subject: [PATCH 08/20] style --- R/oc_process.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/oc_process.R b/R/oc_process.R index bff8b1b7..253f69f9 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -256,7 +256,7 @@ oc_get <- function(oc_url_parts = NULL) { query_req <- build_query_with_req(oc_url_parts) query_req %>% - httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L)/1L) %>% + httr2::req_throttle(rate = getOption("oc_rate_sec", default = 1L) / 1L) %>% httr2::req_user_agent(oc_ua_string) %>% httr2::req_perform() # will error if API error :-) } From 98483336a598847eb416a7613e907b83f93cf239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:07:35 +0100 Subject: [PATCH 09/20] space --- R/oc_process.R | 1 - 1 file changed, 1 deletion(-) diff --git a/R/oc_process.R b/R/oc_process.R index 253f69f9..95fd6f65 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -270,7 +270,6 @@ build_query_with_req <- function(oc_url_parts) { initial_req } - if (!is.null(oc_url_parts[["query"]])) { # some gymnastics needed as we can't pass the query as a list to httr2? args <- oc_url_parts[["query"]] From 4325eed554f3401cdbdedebbffa97dcab23f281b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:14:12 +0100 Subject: [PATCH 10/20] fix test --- tests/testthat/test-oc_check_status.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-oc_check_status.R b/tests/testthat/test-oc_check_status.R index 23e0bda0..7bd544e4 100644 --- a/tests/testthat/test-oc_check_status.R +++ b/tests/testthat/test-oc_check_status.R @@ -22,7 +22,7 @@ test_that("oc_check_status returns 400 error if request is invalid", { longitude = 0, return = "json_list" ), - "HTTP failure: 400" + "HTTP 400 Bad Request" ) # We don't send queries with nchar(query) <= 1 to the API, see .oc_process() @@ -31,7 +31,7 @@ test_that("oc_check_status returns 400 error if request is invalid", { placename = " ", return = "json_list" ), - "HTTP failure: 400" + "HTTP 400 Bad Request" ) }) @@ -42,7 +42,7 @@ test_that("oc_check_status returns 401 error if key is invalid", { withr::local_envvar(c("OPENCAGE_KEY" = "32charactersandnumbers1234567890")) expect_error( oc_reverse(latitude = 0, longitude = 0), - "HTTP failure: 401" + "HTTP 401 Unauthorized" ) }) @@ -53,7 +53,7 @@ test_that("oc_check_status returns 402 error if quota exceeded", { withr::local_envvar(c("OPENCAGE_KEY" = key_402)) expect_error( oc_reverse(latitude = 0, longitude = 0), - "HTTP failure: 402" + "HTTP 402" ) }) @@ -64,7 +64,7 @@ test_that("oc_check_status returns 403 error if key is blocked", { withr::local_envvar(c("OPENCAGE_KEY" = key_403)) expect_error( oc_reverse(latitude = 0, longitude = 0), - "HTTP failure: 403" + "HTTP 403" ) }) @@ -75,6 +75,6 @@ test_that("oc_check_status returns 429 error if rate limit is exceeded", { withr::local_envvar(c("OPENCAGE_KEY" = key_429)) expect_error( oc_reverse(latitude = 0, longitude = 0), - "HTTP failure: 429" + "HTTP 429" ) }) From 62377e30a3b30f5c0e276054f830e5a7d3815bca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:14:19 +0100 Subject: [PATCH 11/20] simpler code --- R/oc_process.R | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/R/oc_process.R b/R/oc_process.R index 95fd6f65..8f576ee8 100644 --- a/R/oc_process.R +++ b/R/oc_process.R @@ -270,14 +270,10 @@ build_query_with_req <- function(oc_url_parts) { initial_req } - if (!is.null(oc_url_parts[["query"]])) { - # some gymnastics needed as we can't pass the query as a list to httr2? - args <- oc_url_parts[["query"]] - args[[".req"]] <- req_with_url - - query_req <- do.call(what = httr2::req_url_query, args = args) + query_req <- if (!is.null(oc_url_parts[["query"]])) { + httr2::req_url_query(req_with_url, !!!oc_url_parts[["query"]]) } else { - query_req <- req_with_url + req_with_url } query_req From 81862dd244d6b62b6b9ca5c99c50766d1f2e2c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:22:41 +0100 Subject: [PATCH 12/20] 1 fix --- tests/testthat/test-oc_get.R | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/testthat/test-oc_get.R b/tests/testthat/test-oc_get.R index f88cd732..f7d8ec4b 100644 --- a/tests/testthat/test-oc_get.R +++ b/tests/testthat/test-oc_get.R @@ -14,7 +14,7 @@ test_that("oc_get returns a response object", { endpoint = "json" ) ), - "HttpResponse" + "httr2_response" ) }) @@ -28,12 +28,13 @@ test_that("oc_get returns a response object for Namibia NA countrycode", { query_par = list( placename = "Windhoek", key = Sys.getenv("OPENCAGE_KEY"), - countrycode = "NA" + countrycode = "NA", + limit = 2 ), endpoint = "json" ) ), - "HttpResponse" + "httr2_response" ) }) @@ -52,7 +53,7 @@ test_that("oc_get returns a response object for vector countrycode", { endpoint = "json" ) ), - "HttpResponse" + "httr2_response" ) }) From a2f527bb0a7fa27940394faa18fd7a9e00940eb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Fri, 3 Feb 2023 09:29:22 +0100 Subject: [PATCH 13/20] didn't work --- tests/testthat/test-oc_get.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/testthat/test-oc_get.R b/tests/testthat/test-oc_get.R index f7d8ec4b..497cb77e 100644 --- a/tests/testthat/test-oc_get.R +++ b/tests/testthat/test-oc_get.R @@ -28,8 +28,7 @@ test_that("oc_get returns a response object for Namibia NA countrycode", { query_par = list( placename = "Windhoek", key = Sys.getenv("OPENCAGE_KEY"), - countrycode = "NA", - limit = 2 + countrycode = "NA" ), endpoint = "json" ) From 957377ea6ea56c796139cc74f150853e73350898 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Tue, 4 Apr 2023 15:24:29 +0200 Subject: [PATCH 14/20] re-add --- R/oc_config.R | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/R/oc_config.R b/R/oc_config.R index 08b3fc16..65976fc8 100644 --- a/R/oc_config.R +++ b/R/oc_config.R @@ -93,6 +93,7 @@ oc_config <- function(key = Sys.getenv("OPENCAGE_KEY"), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), + rate_sec = getOption("oc_rate_sec", default = 1L), ...) { key_needed <- c( @@ -124,4 +125,10 @@ oc_config <- # set show_key options("oc_show_key" = show_key) + + # set rate + if (!is.numeric(rate_sec)) { + cli::cli_abort("Must use a numeric {.code rate_sec}") + } + options("oc_rate_sec" = rate_sec) } From b2b823ffe573e64f7c116cbabadadc9bfa870df4 Mon Sep 17 00:00:00 2001 From: maelle Date: Tue, 4 Apr 2023 13:28:13 +0000 Subject: [PATCH 15/20] Update documentation --- man/oc_config.Rd | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/man/oc_config.Rd b/man/oc_config.Rd index e79c84c4..5c57f3c8 100644 --- a/man/oc_config.Rd +++ b/man/oc_config.Rd @@ -8,6 +8,7 @@ oc_config( key = Sys.getenv("OPENCAGE_KEY"), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), + rate_sec = getOption("oc_rate_sec", default = 1L), ... ) } @@ -27,11 +28,11 @@ not \code{TRUE}, the API key will be replaced with the string \code{OPENCAGE_KEY \code{show_key} defaults to the value set in the \code{oc_show_key} option, or, in case that does not exist, to \code{FALSE}.} -\item{...}{Ignored.} - \item{rate_sec}{Numeric vector of length one. Sets the maximum number of requests sent to the OpenCage API per second. Defaults to the value set in the \code{oc_rate_sec} option, or, in case that does not exist, to 1L.} + +\item{...}{Ignored.} } \description{ Configure session settings for \pkg{opencage}. From 9e08f5f30ff6040082ec9accdc9e7bed90cd5c6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Tue, 4 Apr 2023 15:31:42 +0200 Subject: [PATCH 16/20] set minimal version for httr2 --- DESCRIPTION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index bbaad734..fccc4013 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -26,7 +26,7 @@ Depends: R (>= 3.4.0) Imports: dplyr (>= 0.7.4), - httr2, + httr2 (>= 0.2.0), jsonlite (>= 1.5), lifecycle, magrittr, From 23cab1d8608d10fb01e8f122974f4eb8207f14b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Tue, 4 Apr 2023 15:40:40 +0200 Subject: [PATCH 17/20] Update tests/testthat/test-oc_clear_cache.R Co-authored-by: Daniel Possenriede --- tests/testthat/test-oc_clear_cache.R | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/testthat/test-oc_clear_cache.R b/tests/testthat/test-oc_clear_cache.R index e1131618..657340ca 100644 --- a/tests/testthat/test-oc_clear_cache.R +++ b/tests/testthat/test-oc_clear_cache.R @@ -2,9 +2,6 @@ test_that("oc_clear_cache clears cache", { skip_on_cran() skip_if_offline("httpbin.org") - # until a memoise >v.1.1 is released, we need to run oc_get_memoise() twice to - # have it really cache results - # https://github.com/ropensci/opencage/pull/87#issuecomment-573573183 replicate(2, oc_get_memoise()) expect_true(memoise::has_cache(oc_get_memoise)()) oc_clear_cache() From f4f0f856cc4794312b8dfc77a545cdb8b3fb56c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Tue, 4 Apr 2023 15:47:42 +0200 Subject: [PATCH 18/20] add test --- tests/testthat/test-oc_config.R | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/testthat/test-oc_config.R b/tests/testthat/test-oc_config.R index 7f44c932..5d673f9e 100644 --- a/tests/testthat/test-oc_config.R +++ b/tests/testthat/test-oc_config.R @@ -93,3 +93,12 @@ test_that("oc_config sets show_key option", { oc_config(show_key = TRUE) expect_true(getOption("oc_show_key")) }) + +test_that("rate_sec checks/sets oc_rate_sec option", { + withr::local_envvar(c("OPENCAGE_KEY" = key_200)) + withr::local_options(oc_rate_sec = 123) + oc_config(rate_sec = 42) + expect_equal(getOption("oc_rate_sec"), 42) + + expect_error(oc_config(rate_sec = "blablabla"), "Must") +}) From 81e6b304aabb481a895fce090b3714baf494fc15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABlle=20Salmon?= Date: Tue, 4 Apr 2023 15:49:43 +0200 Subject: [PATCH 19/20] do not change arg order :facepalm: --- R/oc_config.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/oc_config.R b/R/oc_config.R index 65976fc8..ebd9b73a 100644 --- a/R/oc_config.R +++ b/R/oc_config.R @@ -91,9 +91,9 @@ #' @export oc_config <- function(key = Sys.getenv("OPENCAGE_KEY"), + rate_sec = getOption("oc_rate_sec", default = 1L), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), - rate_sec = getOption("oc_rate_sec", default = 1L), ...) { key_needed <- c( From 0b0f440019bd958f75bd772e8933e8dbbe3b8d51 Mon Sep 17 00:00:00 2001 From: maelle Date: Tue, 4 Apr 2023 13:53:00 +0000 Subject: [PATCH 20/20] Update documentation --- man/oc_config.Rd | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/man/oc_config.Rd b/man/oc_config.Rd index 5c57f3c8..c20e23bd 100644 --- a/man/oc_config.Rd +++ b/man/oc_config.Rd @@ -6,9 +6,9 @@ \usage{ oc_config( key = Sys.getenv("OPENCAGE_KEY"), + rate_sec = getOption("oc_rate_sec", default = 1L), no_record = getOption("oc_no_record", default = TRUE), show_key = getOption("oc_show_key", default = FALSE), - rate_sec = getOption("oc_rate_sec", default = 1L), ... ) } @@ -16,6 +16,10 @@ oc_config( \item{key}{Your OpenCage API key as a character vector of length one. Do not pass the key directly as a parameter, though. See details.} +\item{rate_sec}{Numeric vector of length one. Sets the maximum number of +requests sent to the OpenCage API per second. Defaults to the value set in +the \code{oc_rate_sec} option, or, in case that does not exist, to 1L.} + \item{no_record}{Logical vector of length one. When \code{TRUE}, OpenCage will not create log entries of the queries and will not cache the geocoding requests. Defaults to the value set in the \code{oc_no_record} option, or, in @@ -28,10 +32,6 @@ not \code{TRUE}, the API key will be replaced with the string \code{OPENCAGE_KEY \code{show_key} defaults to the value set in the \code{oc_show_key} option, or, in case that does not exist, to \code{FALSE}.} -\item{rate_sec}{Numeric vector of length one. Sets the maximum number of -requests sent to the OpenCage API per second. Defaults to the value set in -the \code{oc_rate_sec} option, or, in case that does not exist, to 1L.} - \item{...}{Ignored.} } \description{