Nicer error message in coord-* functions (#4894)

92amartins · web-flow · commit 23508b321357 · 2023-06-23T21:50:16.000+02:00
* Add `check_coord_limits` utility function

* Use `check_coord_limits` in all `coord-*` functions

* Update NEWS.md

* Update test-coord-* comments to better reflect expected behavior
diff --git a/NEWS.md b/NEWS.md
@@ -1,5 +1,8 @@
 # ggplot2 (development version)
 
+* Nicer error messages for xlim/ylim arguments in coord-* functions
+  (@92amartins, #4601, #5297).
+
 * `coord_sf()` now uses customisable guides provided in the scales or 
   `guides()` function (@teunbrand).
 
diff --git a/R/coord-.R b/R/coord-.R
@@ -218,3 +218,22 @@ render_axis <- function(panel_params, axis, scale, position, theme) {
     zeroGrob()
   }
 }
+
+# Utility function to check coord limits
+check_coord_limits <- function(
+    limits, arg = caller_arg(limits), call = caller_env()
+) {
+  if (is.null(limits)) {
+    return(invisible(NULL))
+  }
+  if (!obj_is_vector(limits) || length(limits) != 2) {
+    what <- "{.obj_type_friendly {limits}}"
+    if (is.vector(limits)) {
+      what <- paste0(what, " of length {length(limits)}")
+    }
+    cli::cli_abort(
+      paste0("{.arg {arg}} must be a vector of length 2, not ", what, "."),
+      call = call
+    )
+  }
+}
diff --git a/R/coord-cartesian-.R b/R/coord-cartesian-.R
@@ -61,6 +61,8 @@
 #' d + coord_cartesian(xlim = c(0, 1))
 coord_cartesian <- function(xlim = NULL, ylim = NULL, expand = TRUE,
                             default = FALSE, clip = "on") {
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
   ggproto(NULL, CoordCartesian,
     limits = list(x = xlim, y = ylim),
     expand = expand,
diff --git a/R/coord-fixed.R b/R/coord-fixed.R
@@ -23,6 +23,8 @@
 #'
 #' # Resize the plot to see that the specified aspect ratio is maintained
 coord_fixed <- function(ratio = 1, xlim = NULL, ylim = NULL, expand = TRUE, clip = "on") {
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
   ggproto(NULL, CoordFixed,
     limits = list(x = xlim, y = ylim),
     ratio = ratio,
diff --git a/R/coord-flip.R b/R/coord-flip.R
@@ -44,6 +44,8 @@
 #'   geom_area() +
 #'   coord_flip()
 coord_flip <- function(xlim = NULL, ylim = NULL, expand = TRUE, clip = "on") {
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
   ggproto(NULL, CoordFlip,
     limits = list(x = xlim, y = ylim),
     expand = expand,
diff --git a/R/coord-map.R b/R/coord-map.R
@@ -135,6 +135,9 @@ coord_map <- function(projection="mercator", ..., parameters = NULL, orientation
     params <- parameters
   }
 
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
+
   ggproto(NULL, CoordMap,
     projection = projection,
     orientation = orientation,
diff --git a/R/coord-quickmap.R b/R/coord-quickmap.R
@@ -2,6 +2,8 @@
 #' @export
 #' @rdname coord_map
 coord_quickmap <- function(xlim = NULL, ylim = NULL, expand = TRUE, clip = "on") {
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
   ggproto(NULL, CoordQuickmap,
     limits = list(x = xlim, y = ylim),
     expand = expand,
diff --git a/R/coord-sf.R b/R/coord-sf.R
@@ -559,6 +559,9 @@ coord_sf <- function(xlim = NULL, ylim = NULL, expand = TRUE,
     lims_method <- arg_match0(lims_method, c("cross", "box", "orthogonal", "geometry_bbox"))
   }
 
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
+
   ggproto(NULL, CoordSf,
     limits = list(x = xlim, y = ylim),
     lims_method = lims_method,
diff --git a/R/coord-transform.R b/R/coord-transform.R
@@ -86,6 +86,9 @@ coord_trans <- function(x = "identity", y = "identity", xlim = NULL, ylim = NULL
     ylim <- limy
   }
 
+  check_coord_limits(xlim)
+  check_coord_limits(ylim)
+
   # resolve transformers
   if (is.character(x)) x <- as.trans(x)
   if (is.character(y)) y <- as.trans(y)
diff --git a/tests/testthat/_snaps/coord-cartesian.md b/tests/testthat/_snaps/coord-cartesian.md
@@ -0,0 +1,8 @@
+# cartesian coords throws error when limits are badly specified
+
+    `xlim` must be a vector of length 2, not a <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
+
+---
+
+    `ylim` must be a vector of length 2, not an integer vector of length 3.
+
diff --git a/tests/testthat/_snaps/coord-flip.md b/tests/testthat/_snaps/coord-flip.md
@@ -0,0 +1,8 @@
+# flip coords throws error when limits are badly specified
+
+    `xlim` must be a vector of length 2, not a <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
+
+---
+
+    `ylim` must be a vector of length 2, not an integer vector of length 3.
+
diff --git a/tests/testthat/_snaps/coord-map.md b/tests/testthat/_snaps/coord-map.md
@@ -0,0 +1,8 @@
+# coord map throws error when limits are badly specified
+
+    `xlim` must be a vector of length 2, not a <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
+
+---
+
+    `ylim` must be a vector of length 2, not an integer vector of length 3.
+
diff --git a/tests/testthat/_snaps/coord-transform.md b/tests/testthat/_snaps/coord-transform.md
@@ -0,0 +1,8 @@
+# coord_trans() throws error when limits are badly specified
+
+    `xlim` must be a vector of length 2, not a <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
+
+---
+
+    `ylim` must be a vector of length 2, not an integer vector of length 3.
+
diff --git a/tests/testthat/_snaps/coord_sf.md b/tests/testthat/_snaps/coord_sf.md
@@ -19,3 +19,11 @@
     Scale limits cannot be mapped onto spatial coordinates in `coord_sf()`
     i Consider setting `lims_method = "geometry_bbox"` or `default_crs = NULL`.
 
+# coord_sf() throws error when limits are badly specified
+
+    `xlim` must be a vector of length 2, not a <ScaleContinuousPosition/ScaleContinuous/Scale/ggproto/gg> object.
+
+---
+
+    `ylim` must be a vector of length 2, not an integer vector of length 3.
+
diff --git a/tests/testthat/test-coord-.R b/tests/testthat/test-coord-.R
@@ -40,3 +40,16 @@ test_that("guide names are not removed by `train_panel_guides()`", {
   expect_equal(names(layout$panel_params[[1]]$guides$aesthetics),
                c("x", "y", "x.sec", "y.sec"))
 })
+
+test_that("check coord limits errors only on bad inputs", {
+  # Should return NULL if valid values are passed
+  expect_null(check_coord_limits(NULL))
+  expect_null(check_coord_limits(1:2))
+  expect_null(check_coord_limits(c(1,2)))
+
+  # Should raise error if Scale object is passed
+  expect_error(check_coord_limits(xlim(1,2)))
+
+  # Should raise error if vector of wrong length is passed
+  expect_error(check_coord_limits(1:3))
+})
diff --git a/tests/testthat/test-coord-cartesian.R b/tests/testthat/test-coord-cartesian.R
@@ -14,6 +14,14 @@ test_that("clipping can be turned off and on", {
   expect_equal(coord$clip, "on")
 })
 
+test_that("cartesian coords throws error when limits are badly specified", {
+  # throws error when limit is a Scale object instead of vector
+  expect_snapshot_error(ggplot() + coord_cartesian(xlim(1,1)))
+
+  # throws error when limit's length is different than two
+  expect_snapshot_error(ggplot() + coord_cartesian(ylim=1:3))
+})
+
 
 # Visual tests ------------------------------------------------------------
 
diff --git a/tests/testthat/test-coord-flip.R b/tests/testthat/test-coord-flip.R
@@ -7,3 +7,11 @@ test_that("secondary labels are correctly turned off", {
       coord_flip()
   )
 })
+
+test_that("flip coords throws error when limits are badly specified", {
+  # throws error when limit is a Scale object instead of vector
+  expect_snapshot_error(ggplot() + coord_flip(xlim(1,1)))
+
+  # throws error when limit's length is different than two
+  expect_snapshot_error(ggplot() + coord_flip(ylim=1:3))
+})
diff --git a/tests/testthat/test-coord-map.R b/tests/testthat/test-coord-map.R
@@ -42,3 +42,11 @@ test_that("Inf is squished to range", {
   expect_equal(d[[2]]$x, 0)
   expect_equal(d[[2]]$y, 1)
 })
+
+test_that("coord map throws error when limits are badly specified", {
+  # throws error when limit is a Scale object instead of vector
+  expect_snapshot_error(ggplot() + coord_map(xlim=xlim(1,1)))
+
+  # throws error when limit's length is different than two
+  expect_snapshot_error(ggplot() + coord_cartesian(ylim=1:3))
+})
diff --git a/tests/testthat/test-coord-transform.R b/tests/testthat/test-coord-transform.R
@@ -123,3 +123,11 @@ test_that("second axes display in coord_trans()", {
       coord_trans(y = "log2")
   )
 })
+
+test_that("coord_trans() throws error when limits are badly specified", {
+  # throws error when limit is a Scale object instead of vector
+  expect_snapshot_error(ggplot() + coord_trans(xlim=xlim(1,1)))
+
+  # throws error when limit's length is different than two
+  expect_snapshot_error(ggplot() + coord_trans(ylim=1:3))
+})
diff --git a/tests/testthat/test-coord_sf.R b/tests/testthat/test-coord_sf.R
@@ -322,3 +322,11 @@ test_that("coord_sf() uses the guide system", {
     p
   )
 })
+
+test_that("coord_sf() throws error when limits are badly specified", {
+  # throws error when limit is a Scale object instead of vector
+  expect_snapshot_error(ggplot() + coord_sf(xlim(1,1)))
+
+  # throws error when limit's length is different than two
+  expect_snapshot_error(ggplot() + coord_sf(ylim=1:3))
+})

Original file line number	Diff line number	Diff line change
`@@ -135,6 +135,9 @@ coord_map <- function(projection="mercator", ..., parameters = NULL, orientation`
`135`	`135`	`params <- parameters`
`136`	`136`	`}`
`137`	`137`
	`138`	`+ check_coord_limits(xlim)`
	`139`	`+ check_coord_limits(ylim)`
	`140`	`+`
`138`	`141`	`ggproto(NULL, CoordMap,`
`139`	`142`	`projection = projection,`
`140`	`143`	`orientation = orientation,`
Original file line number	Diff line number	Diff line change
`@@ -559,6 +559,9 @@ coord_sf <- function(xlim = NULL, ylim = NULL, expand = TRUE,`
`559`	`559`	`lims_method <- arg_match0(lims_method, c("cross", "box", "orthogonal", "geometry_bbox"))`
`560`	`560`	`}`
`561`	`561`
	`562`	`+ check_coord_limits(xlim)`
	`563`	`+ check_coord_limits(ylim)`
	`564`	`+`
`562`	`565`	`ggproto(NULL, CoordSf,`
`563`	`566`	`limits = list(x = xlim, y = ylim),`
`564`	`567`	`lims_method = lims_method,`
Original file line number	Diff line number	Diff line change
`@@ -86,6 +86,9 @@ coord_trans <- function(x = "identity", y = "identity", xlim = NULL, ylim = NULL`
`86`	`86`	`ylim <- limy`
`87`	`87`	`}`
`88`	`88`
	`89`	`+ check_coord_limits(xlim)`
	`90`	`+ check_coord_limits(ylim)`
	`91`	`+`
`89`	`92`	`# resolve transformers`
`90`	`93`	`if (is.character(x)) x <- as.trans(x)`
`91`	`94`	`if (is.character(y)) y <- as.trans(y)`