From 23508b321357502d014e52b8118ed1b0f9c5c164 Mon Sep 17 00:00:00 2001
From: Andrei <92amartins@gmail.com>
Date: Fri, 23 Jun 2023 16:50:16 -0300
Subject: [PATCH] Nicer error message in coord-* functions (#4894)

* 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
---
 NEWS.md                                  |  3 +++
 R/coord-.R                               | 19 +++++++++++++++++++
 R/coord-cartesian-.R                     |  2 ++
 R/coord-fixed.R                          |  2 ++
 R/coord-flip.R                           |  2 ++
 R/coord-map.R                            |  3 +++
 R/coord-quickmap.R                       |  2 ++
 R/coord-sf.R                             |  3 +++
 R/coord-transform.R                      |  3 +++
 tests/testthat/_snaps/coord-cartesian.md |  8 ++++++++
 tests/testthat/_snaps/coord-flip.md      |  8 ++++++++
 tests/testthat/_snaps/coord-map.md       |  8 ++++++++
 tests/testthat/_snaps/coord-transform.md |  8 ++++++++
 tests/testthat/_snaps/coord_sf.md        |  8 ++++++++
 tests/testthat/test-coord-.R             | 13 +++++++++++++
 tests/testthat/test-coord-cartesian.R    |  8 ++++++++
 tests/testthat/test-coord-flip.R         |  8 ++++++++
 tests/testthat/test-coord-map.R          |  8 ++++++++
 tests/testthat/test-coord-transform.R    |  8 ++++++++
 tests/testthat/test-coord_sf.R           |  8 ++++++++
 20 files changed, 132 insertions(+)
 create mode 100644 tests/testthat/_snaps/coord-cartesian.md
 create mode 100644 tests/testthat/_snaps/coord-flip.md
 create mode 100644 tests/testthat/_snaps/coord-map.md
 create mode 100644 tests/testthat/_snaps/coord-transform.md

diff --git a/NEWS.md b/NEWS.md
index 7a9eb66130..6fb136dcfc 100644
--- 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
index 46deae8444..8944c812ee 100644
--- 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
index cd68e36eef..7c64ec9744 100644
--- 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
index 73015e7921..a942fbb28b 100644
--- 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
index 8ea5614580..1f3848fb8a 100644
--- 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
index 261a74f60e..0b0744c1b0 100644
--- 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
index 0e7971f350..31367367a6 100644
--- 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
index 318c0159ca..81207f2ce1 100644
--- 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
index 7cb08e5b30..5beaadf6d2 100644
--- 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
new file mode 100644
index 0000000000..7da67ba9c9
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..b7717a7381
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..2afa61e0a7
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..14be4bd125
--- /dev/null
+++ 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
index 091732b2fa..c53025f074 100644
--- 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
index c69eab0b51..ca7f62c06e 100644
--- 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
index 5b0ea913d3..23bed331ae 100644
--- 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
index c3a24300b6..0a346ebb24 100644
--- 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
index e9b6185e71..9805eaf5a8 100644
--- 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
index 1e2f882f87..559ea74e88 100644
--- 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
index 640915599d..90fbcb333b 100644
--- 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))
+})