refactor: replace {crul} with {httr2} by maelle · Pull Request #157 · ropensci/opencage

maelle · 2023-02-02T15:06:03Z

maelle · 2023-02-02T15:08:41Z

also need to use https://httr2.r-lib.org/reference/req_throttle.html

dpprdan · 2023-02-02T15:20:48Z

Yeah, watch out for https://github.com/ropensci/opencage/blob/main/R/zzz.R and

Lines 122 to 126 in 9b57eef

    
           # set rate limit 
        
           ratelimitr::UPDATE_RATE( 
        
             oc_get_limited, 
        
             ratelimitr::rate(n = rate_sec, period = 1L) 
        
           )

Though I am sure you've seen that.

maelle · 2023-02-03T08:05:45Z

R/oc_config.R

 #' @export
 oc_config <-
  function(key = Sys.getenv("OPENCAGE_KEY"),
-           rate_sec = getOption("oc_rate_sec", default = 1L),


maybe needs actual deprecation 😅

I'd rather leave it in here (so all possible opencage session level options can be set with oc_config()) and just set options("oc_rate_sec" = rate_sec), like no_record and show_key.
Maybe check whether rate_sec is numeric beforehand.

R/oc_process.R

maelle · 2023-02-03T08:25:13Z

tests/testthat/test-oc_get.R

      )
    ),
-    "HttpResponse"
+    "httr2_response"


these tests are actually failing with "HTTP 400 Bad Request". I'm not sure why as the same oc_forward() query works. I see the first test in this file, that doesn't fail, uses a key for always getting 200. I'm a bit lost.

All queries/tests in test-oc_get.R return HTTP 400 errors, also on main, because the query parameter placename ought to be q here 🙈. Well, the first one passes, because it uses the key_200 which always returns a HTTP 200 response, no matter what nonsense you throw at the API.
I don't know whether this has always been the case or whether there has been a related change in the OpenCage API. Regardless, it never surfaced, because all queries only test whether a HttpResponse object is returned by oc_get(), and the status wasn't checked until later. But the status check is now as part of oc_get() so, the error now surfaces here already.

I am not sure about the usefulness of the Namibia and vector countrycode tests here. I think that these should be tested with oc_forward() if they aren't already?

Here, I am more interested in whether memoisation and rate limiting works as expected.

As noted in another comment, I wouldn't test memoisation nor rate limiting as we outsource those.

What we should test is as you noted, whether the configuration function can configure the rate. BUT since in req_throttle() I call getOption("oc_rate_sec", default = 1L), it only means retrieving the option before and after calling the configuration function.

Would you agree?

maelle · 2023-02-03T08:31:57Z

tests/testthat/test-oc_clear_cache.R

  # 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"))


not sure these edits were the right ones. However now one cannot "get" stuff from an URL that's not the OpenCage API, with the refactoring.

I am not entirely sure either whether this actually tests what we want to.
Could we build oc_get() in such a way that it accepts a httr2 request object and just performs the request (i.e. just wrapper around httr2::req_perform())? Then we could just build a simple query request here (I'd rather not hit the OpenCage API for this).

I am now wondering why we are testing memoization at all, given it's "outsourced" to {memoise}?

I'm also not convinced we need to test oc_clear_cache() as it only wraps memoise that we trust?

By the way here's another caching method: https://deploy-preview-537--ropensci.netlify.app/blog/2023/04/21/ropensci-news-digest-april-2023/#caching-the-results-of-functions-of-your-r-package

dpprdan

Sorry, quite some comments. 🙈 I hope they are at least comprehensible.

dpprdan · 2023-02-08T16:51:16Z

R/oc_config.R

 #' @export
 oc_config <-
  function(key = Sys.getenv("OPENCAGE_KEY"),
-           rate_sec = getOption("oc_rate_sec", default = 1L),


I'd rather leave it in here (so all possible opencage session level options can be set with oc_config()) and just set options("oc_rate_sec" = rate_sec), like no_record and show_key.
Maybe check whether rate_sec is numeric beforehand.

dpprdan · 2023-02-08T16:53:16Z

tests/testthat/test-oc_get.R

      )
    ),
-    "HttpResponse"
+    "httr2_response"


All queries/tests in test-oc_get.R return HTTP 400 errors, also on main, because the query parameter placename ought to be q here 🙈. Well, the first one passes, because it uses the key_200 which always returns a HTTP 200 response, no matter what nonsense you throw at the API.
I don't know whether this has always been the case or whether there has been a related change in the OpenCage API. Regardless, it never surfaced, because all queries only test whether a HttpResponse object is returned by oc_get(), and the status wasn't checked until later. But the status check is now as part of oc_get() so, the error now surfaces here already.

I am not sure about the usefulness of the Namibia and vector countrycode tests here. I think that these should be tested with oc_forward() if they aren't already?

Here, I am more interested in whether memoisation and rate limiting works as expected.

dpprdan · 2023-02-08T16:56:55Z

tests/testthat/test-oc_config.R

  )
 })

-# test rate_sec argument --------------------------------------------------


Could you please add a little test whether oc_config() can update the oc_rate_limit option?

dpprdan · 2023-02-08T17:13:51Z

DESCRIPTION

 Imports: 
-    crul (>= 0.5.2),
    dplyr (>= 0.7.4),
+    httr2,


Is v0.1.1 sufficient or do we use a feature from a more recent version?
https://httr2.r-lib.org/news/index.html

(Didn't realise that httr2 is so recent 😮)

I added a dependency on >= 0.2.0 because of r-lib/httr2#101

dpprdan · 2023-02-08T20:21:24Z

R/oc_process.R


    # build url
-    oc_url <- oc_build_url(
+    oc_url_parts <- oc_build_url(


The current naming confused me a little, I am afraid.

I think there are two to three steps until the request is made

1a. format the query parameters, i.e. turn booleans into integers, delete unused parameters, concatenate bounds coordinates, etc. This is now done in oc_build_url() except this does not yet build the url, so maybe oc_format_query_parameters() and the resulting list is query_parameters? I would move the endpoint as a separate input to the next step/function.
1b. build the httr2 request object (or rather just its url part). Basically what build_query_with_req() does now (maybe just name it build_request()?). The inputs are the query_parameters and endpoint, the output is query_req.
2. Make the request with oc_get(). Pretty much like it is now, but with request or similar as an argument name. (Note to self: This has to be a separate step, so we can return the url_only before).

🤔 À propos naming: Maybe I overdid it with the oc_ prefix for internal functions... 😉

dpprdan · 2023-02-08T20:56:34Z

R/oc_process.R

+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"]])) {
+    httr2::req_url_path_append(initial_req, oc_url_parts[["path"]])
+  } else {
+    initial_req
+  }
+
+  query_req <- if (!is.null(oc_url_parts[["query"]])) {
+    httr2::req_url_query(req_with_url, !!!oc_url_parts[["query"]])
+  } else {
+    req_with_url
+  }
+
+  query_req


If we only use this within oc_process() and tie it to the OpenCage API domain, I think the following should be just as safe. I.e. I cannot think of a scenario where we would make queries to api.opencagedata.com without an endpoint or query_parameters.

Suggested change

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"]])) {

httr2::req_url_path_append(initial_req, oc_url_parts[["path"]])

} else {

initial_req

}

query_req <- if (!is.null(oc_url_parts[["query"]])) {

httr2::req_url_query(req_with_url, !!!oc_url_parts[["query"]])

} else {

req_with_url

}

query_req

build_request <- function(endpoint, query_parameters) {

httr2::request("https://api.opencagedata.com") %>%

httr2::req_url_path_append(endpoint) %>%

httr2::req_url_query(req_with_url, !!!query_parameters)

But if we want to keep it generic (e.g. for the oc_get_memoise() tests), we could instead do

Suggested change

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"]])) {

httr2::req_url_path_append(initial_req, oc_url_parts[["path"]])

} else {

initial_req

}

query_req <- if (!is.null(oc_url_parts[["query"]])) {

httr2::req_url_query(req_with_url, !!!oc_url_parts[["query"]])

} else {

req_with_url

}

query_req

build_request <- function(base_url = "https://api.opencagedata.com", endpoint, query_parameters) {

query_req <- httr2::request(base_url)

query_req <- if (!is.null(endpoint)) {

httr2::req_url_path_append(query_req , endpoint)

}

query_req <- if (!is.null(query_parameters)) {

httr2::req_url_query(query_req, !!!query_parameters)

}

query_req

dpprdan · 2023-02-08T20:59:30Z

DESCRIPTION

+    httr2,
    jsonlite (>= 1.5),
    lifecycle,
+    magrittr,


We might as well take the dependency on magrittr v2.0 then?
I.e. does it make sense to more generally bump all versions roughly to the one before the most recent necessary dependency (i.e. httr2) came out? Or is that a mistake? I wouldn't know what more ancient versions really mean practically?

Suggested change

magrittr,

magrittr (>= 2.0.0),

is there a specific reason for depending on that magrittr version?

tests/testthat/test-oc_clear_cache.R

dpprdan · 2023-02-08T21:04:21Z

tests/testthat/test-oc_clear_cache.R

  # 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"))


I am not entirely sure either whether this actually tests what we want to.
Could we build oc_get() in such a way that it accepts a httr2 request object and just performs the request (i.e. just wrapper around httr2::req_perform())? Then we could just build a simple query request here (I'd rather not hit the OpenCage API for this).

dpprdan · 2023-02-08T21:04:55Z

tests/testthat/test-oc_clear_cache.R

  # 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())


Suggested change

replicate(2, oc_get_memoise())

oc_get_memoise()

Co-authored-by: Daniel Possenriede <possenriede@gmail.com>

maelle · 2023-04-04T13:42:25Z

Oh: https://httr2.r-lib.org/reference/req_cache.html (not sure how to clear that cache)

maelle added 2 commits February 2, 2023 15:53

refactor: replace {crul} with {httr2}

4e5ae63

less crul

2fd20d8

maelle added 5 commits February 3, 2023 08:29

some progress

d7e17d0

fixes

fd93371

refactor

10577f9

fix

b257006

ouch

06565c8

maelle commented Feb 3, 2023

View reviewed changes

R/oc_process.R Show resolved Hide resolved

maelle added 5 commits February 3, 2023 09:06

style

023971a

space

9848333

fix test

4325eed

simpler code

62377e3

1 fix

81862dd

maelle commented Feb 3, 2023

View reviewed changes

didn't work

a2f527b

maelle requested a review from dpprdan February 3, 2023 08:30

maelle commented Feb 3, 2023

View reviewed changes

dpprdan requested changes Feb 8, 2023

View reviewed changes

maelle and others added 4 commits April 4, 2023 15:24

re-add

957377e

Update documentation

b2b823f

set minimal version for httr2

9e08f5f

Update tests/testthat/test-oc_clear_cache.R

23cab1d

Co-authored-by: Daniel Possenriede <possenriede@gmail.com>

maelle and others added 3 commits April 4, 2023 15:49

add test

f4f0f85

do not change arg order 🤦

81e6b30

Update documentation

0b0f440

Conversation

maelle commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelle commented Feb 2, 2023

Uh oh!

dpprdan commented Feb 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpprdan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelle commented Apr 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maelle commented Feb 2, 2023 •

edited

Loading