Added a more informative error message when oe_get or oe_read are run…

… with unnamed arguments that are passed down to st_read + more cautious management of ... fix #234
ropensci · Nov 12, 2021 · 79a61bb · 79a61bb
1 parent 33dfdcc
commit 79a61bb
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 16 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -2,6 +2,7 @@
 
 ### MINOR CHANGES
 * The `boundary` argument can be specified using `bbox` objects. The `bbox` object is converted to `sfc` object with `sf::st_as_sfc` and preserves the same CRS. 
+* Added a more informative error message when `oe_get()` or `oe_read()` are run with empty or unnamed arguments in `...` (#234).
 
 ### DOCUMENTATION FIXES
 * Update description for `boundary` and `boundary_type` arguments. 

diff --git a/R/get.R b/R/get.R
@@ -95,9 +95,9 @@
 #' @param download_only Boolean. If `TRUE`, then the function only returns the
 #'   path where the matched file is stored, instead of reading it. `FALSE` by
 #'   default.
-#' @param ... Arguments that will be passed to [`sf::st_read()`], like `query`,
-#'   `wkt_filter` or `stringsAsFactors`.  Check the introductory vignette to
-#'   understand how to create your own (SQL-like) queries.
+#' @param ... (Named) arguments that will be passed to [`sf::st_read()`], like
+#'   `query`, `wkt_filter` or `stringsAsFactors`.  Check the introductory
+#'   vignette to understand how to create your own (SQL-like) queries.
 #'
 #' @return An `sf` object.
 #' @export

diff --git a/R/read.R b/R/read.R
@@ -95,9 +95,21 @@ oe_read = function(
   # Test misspelt arguments
   check_layer_provider(layer, provider)
 
-  # Test if there is misalignment between query and layer. See also
-  # See https://github.com/ropensci/osmextract/issues/122
-  if ("query" %in% names(list(...))) {
+  # Check that all arguments inside ... are named arguments. See also
+  # https://github.com/ropensci/osmextract/issues/234
+  if (...length() && any(is.na(...names()))) {
+    stop(
+      "All arguments in oe_get and oe_read beside 'place' and 'layer' must be named. ",
+      "Moreover, please check that you didn't add an extra comma at the end of your call",
+      call. = FALSE
+    )
+  }
+
+  # Test if there is misalignment between query and layer. See also See
+  # https://github.com/ropensci/osmextract/issues/122. Moreover, use ...names()
+  # instead of names(list(...)) because of
+  # https://github.com/ropensci/osmextract/issues/234
+  if ("query" %in% ...names()) {
     # Check if the query argument defined in sf::st_read was defined using a
     # layer different than layer argument.
     # Extracted from sf::st_read docs: For query with a character dsn the query
@@ -156,7 +168,7 @@ oe_read = function(
     # returns a notes in the R CMD checks related to :::. Hence, I decided to
     # use names(formals("st_read.character", envir = getNamespace("sf")))
     any(
-      names(list(...)) %!in%
+      ...names() %!in%
       # The ... arguments in st_read are passed to st_as_sf so I need to add the
       # formals of st_as_sf.
       # See https://github.com/ropensci/osmextract/issues/152
@@ -171,7 +183,7 @@ oe_read = function(
       "The following arguments are probably misspelled: ",
       paste(
         setdiff(
-          names(list(...)),
+          ...names(),
           union(
             names(formals(get("st_read.character", envir = getNamespace("sf")))),
             names(formals(get("st_as_sf.data.frame", envir = getNamespace("sf"))))
@@ -204,7 +216,7 @@ oe_read = function(
         ...
       ))
     } else {
-      if ("query" %in% names(list(...))) {
+      if ("query" %in% ...names()) {
         return(sf::st_read(
           dsn = file_path,
           quiet = quiet,
@@ -329,7 +341,7 @@ oe_read = function(
       ...
     )
   } else {
-    if ("query" %in% names(list(...))) {
+    if ("query" %in% ...names()) {
       sf::st_read(
         dsn = gpkg_file_path,
         quiet = quiet,

diff --git a/man/oe_get.Rd b/man/oe_get.Rd
diff --git a/man/oe_read.Rd b/man/oe_read.Rd
diff --git a/tests/testthat/test-read.R b/tests/testthat/test-read.R
@@ -223,6 +223,37 @@ test_that("warning when setting boundary and spat/clipsrc options", {
   ))
 })
 
+test_that("oe_read returns an error with unnamed arguments in ...", {
+  # Unnamed argument:
+  expect_error(
+    oe_read(
+      file_path = its_pbf,
+      layer = "lines",
+      "SELECT * FROM lines"
+    ),
+    "All arguments in oe_get and oe_read beside 'place' and 'layer' must be named"
+  )
+
+  # Extra comma:
+  expect_error(
+    oe_read(
+      file_path = its_pbf,
+      layer = "lines",
+    ),
+    "All arguments in oe_get and oe_read beside 'place' and 'layer' must be named"
+  )
+
+  # Named argument + extra comma:
+  expect_error(
+    oe_read(
+      file_path = its_pbf,
+      layer = "lines",
+      query = "SELECT * FROM lines",
+    ),
+    "All arguments in oe_get and oe_read beside 'place' and 'layer' must be named"
+  )
+})
+
 # Clean tempdir
 rm(its_poly)
 file.remove(list.files(tempdir(), pattern = "its-example", full.names = TRUE))