From 52232aa3639e846c0b96311ff394eca6bcffe581 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Thu, 20 Oct 2022 07:54:13 +0000 Subject: [PATCH 1/7] `req_url_path_*()` can handle empty `...` --- R/req-url.R | 17 ++++++++++------- tests/testthat/test-req-url.R | 6 ++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/R/req-url.R b/R/req-url.R index ea560d9e3..42e6db4b9 100644 --- a/R/req-url.R +++ b/R/req-url.R @@ -54,10 +54,7 @@ req_url_query <- function(.req, ...) { req_url_path <- function(req, ...) { check_request(req) path <- paste(..., sep = "/") - - if (!grepl("^/", path)) { - path <- paste0("/", path) - } + path <- path_add_slash(path) req_url(req, url_modify(req$url, path = path)) } @@ -67,14 +64,20 @@ req_url_path <- function(req, ...) { req_url_path_append <- function(req, ...) { check_request(req) path <- paste(..., sep = "/") + path <- path_add_slash(path) url <- url_parse(req$url) + url$path <- paste0(sub("/$", "", url$path), path) + + req_url(req, url_build(url)) +} + +path_add_slash <- function(path) { # Ensure we don't add duplicate /s - if (!grepl("^/", path)) { + if (!is_empty(path) && !grepl("^/", path)) { path <- paste0("/", path) } - url$path <- paste0(sub("/$", "", url$path), path) - req_url(req, url_build(url)) + path } diff --git a/tests/testthat/test-req-url.R b/tests/testthat/test-req-url.R index c85032cec..1ee34e3a1 100644 --- a/tests/testthat/test-req-url.R +++ b/tests/testthat/test-req-url.R @@ -24,6 +24,12 @@ test_that("can append multiple components", { expect_equal(req_url_path_append(req, "a", "b")$url, "http://example.com/x/a/b") }) +test_that("can handle empty path", { + req <- request("http://example.com/x") + expect_equal(req_url_path(req, NULL)$url, "http://example.com") + expect_equal(req_url_path_append(req, NULL)$url, "http://example.com/x") +}) + test_that("can set query params", { req <- request("http://example.com/") expect_equal(req_url_query(req, a = 1, b = 2)$url, "http://example.com/?a=1&b=2") From 8a9f428cf72545ab18e598bb373249c09e5b084c Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Thu, 20 Oct 2022 07:56:25 +0000 Subject: [PATCH 2/7] Add test with empty `...` --- tests/testthat/test-req-url.R | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/testthat/test-req-url.R b/tests/testthat/test-req-url.R index 1ee34e3a1..67636198b 100644 --- a/tests/testthat/test-req-url.R +++ b/tests/testthat/test-req-url.R @@ -26,6 +26,8 @@ test_that("can append multiple components", { test_that("can handle empty path", { req <- request("http://example.com/x") + expect_equal(req_url_path(req)$url, "http://example.com") + expect_equal(req_url_path_append(req)$url, "http://example.com/x") expect_equal(req_url_path(req, NULL)$url, "http://example.com") expect_equal(req_url_path_append(req, NULL)$url, "http://example.com/x") }) From 8024dae909403fa3f2f2614ac8ee1202d06270f8 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Thu, 20 Oct 2022 07:57:26 +0000 Subject: [PATCH 3/7] NEWS --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index cffea75e4..00bc0011a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # httr2 (development version) +* `req_url_path()` and `req_url_path_append()` can handle `NULL` or empty `...` + (@mgirlich, #177). + # httr2 0.2.2 * `curl_translate()` can now handle curl copied from Chrome developer tools From 2b7e1615ac40a977cc70201853b1307e0a61bc56 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Mon, 24 Oct 2022 13:11:36 +0000 Subject: [PATCH 4/7] Can also handle path of length > 1 --- R/req-url.R | 12 +++++------- tests/testthat/test-req-url.R | 9 +++++++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/R/req-url.R b/R/req-url.R index 42e6db4b9..952883825 100644 --- a/R/req-url.R +++ b/R/req-url.R @@ -53,8 +53,7 @@ req_url_query <- function(.req, ...) { #' @rdname req_url req_url_path <- function(req, ...) { check_request(req) - path <- paste(..., sep = "/") - path <- path_add_slash(path) + path <- dots_to_path(...) req_url(req, url_modify(req$url, path = path)) } @@ -63,19 +62,18 @@ req_url_path <- function(req, ...) { #' @rdname req_url req_url_path_append <- function(req, ...) { check_request(req) - path <- paste(..., sep = "/") - path <- path_add_slash(path) + path <- dots_to_path(...) url <- url_parse(req$url) - url$path <- paste0(sub("/$", "", url$path), path) req_url(req, url_build(url)) } -path_add_slash <- function(path) { +dots_to_path <- function(...) { + path <- paste(c(...), sep = "/", collapse = "/") # Ensure we don't add duplicate /s - if (!is_empty(path) && !grepl("^/", path)) { + if (path != "" && !grepl("^/", path)) { path <- paste0("/", path) } diff --git a/tests/testthat/test-req-url.R b/tests/testthat/test-req-url.R index 67636198b..88b259147 100644 --- a/tests/testthat/test-req-url.R +++ b/tests/testthat/test-req-url.R @@ -30,6 +30,15 @@ test_that("can handle empty path", { expect_equal(req_url_path_append(req)$url, "http://example.com/x") expect_equal(req_url_path(req, NULL)$url, "http://example.com") expect_equal(req_url_path_append(req, NULL)$url, "http://example.com/x") + + expect_equal(req_url_path(req, "")$url, "http://example.com") + expect_equal(req_url_path_append(req, "")$url, "http://example.com/x") +}) + +test_that("can path vector", { + req <- request("http://example.com/x") + expect_equal(req_url_path(req, c("a", "b"))$url, "http://example.com/a/b") + expect_equal(req_url_path_append(req, c("a", "b"))$url, "http://example.com/x/a/b") }) test_that("can set query params", { From 18fccb856121f36fef78b71d4f59bdeb62031a19 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Mon, 24 Oct 2022 13:16:08 +0000 Subject: [PATCH 5/7] Add another test --- tests/testthat/test-req-url.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-req-url.R b/tests/testthat/test-req-url.R index 88b259147..a20438e95 100644 --- a/tests/testthat/test-req-url.R +++ b/tests/testthat/test-req-url.R @@ -35,10 +35,11 @@ test_that("can handle empty path", { expect_equal(req_url_path_append(req, "")$url, "http://example.com/x") }) -test_that("can path vector", { +test_that("can handle path vector", { req <- request("http://example.com/x") expect_equal(req_url_path(req, c("a", "b"))$url, "http://example.com/a/b") expect_equal(req_url_path_append(req, c("a", "b"))$url, "http://example.com/x/a/b") + expect_equal(req_url_path_append(req, c("a", "b"), NULL)$url, "http://example.com/x/a/b") }) test_that("can set query params", { From 659392d4bf286346661a897e35d3bbf38512e40b Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Tue, 25 Oct 2022 06:53:57 +0000 Subject: [PATCH 6/7] Remove unnecessary use of `sep` argument --- R/req-url.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/req-url.R b/R/req-url.R index 952883825..aedac3788 100644 --- a/R/req-url.R +++ b/R/req-url.R @@ -71,7 +71,7 @@ req_url_path_append <- function(req, ...) { } dots_to_path <- function(...) { - path <- paste(c(...), sep = "/", collapse = "/") + path <- paste(c(...), collapse = "/") # Ensure we don't add duplicate /s if (path != "" && !grepl("^/", path)) { path <- paste0("/", path) From ecf28c41aa724bbe44677458816bcc625bcc0e6b Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Thu, 12 Jan 2023 14:33:24 +0000 Subject: [PATCH 7/7] Update NEWS --- NEWS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index eb79e82f5..da7295114 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,7 @@ # httr2 (development version) -* `req_url_path()` and `req_url_path_append()` can handle `NULL` or empty `...` - (@mgirlich, #177). +* `req_url_path()` and `req_url_path_append()` can now handle `NULL` or empty + `...` and the elements of `...` can also have length > 1 (@mgirlich, #177). * `req_perform()` now has an `error_call` argument and communicates more clearly where the error occurred (@mgirlich, #187).