Fix bug in discrete position scales with continuous limits (fixes #3918) (#3919)

* fix manual limits with discrete scale that was only trained using continuous values

* add test for the case of no discrete limits

* add warning if user supplies continuous limits to a discrete scale

* move warning to discrete_scale()

* add test for continuous limits

Author: paleolimbot

R/scale-.r

@@ -164,6 +164,16 @@ discrete_scale <- function(aesthetics, scale_name, palette, name = waiver(),
 
   check_breaks_labels(breaks, labels)
 
+  if (!is.function(limits) && (length(limits) > 0) && !is.discrete(limits)) {
+    warn(
+      glue(
+        "
+        Continuous limits supplied to discrete scale.
+        Did you mean `limits = factor(...)` or `scale_*_continuous()`?"
+      )
+    )
+  }
+
   position <- match.arg(position, c("left", "right", "top", "bottom"))
 
   # If the scale is non-positional, break = NULL means removing the guide

R/scale-expansion.r

@@ -200,21 +200,25 @@ expand_limits_continuous_trans <- function(limits, expand = expansion(0, 0),
 expand_limits_discrete_trans <- function(limits, expand = expansion(0, 0),
                                          coord_limits = c(NA, NA), trans = identity_trans(),
                                          range_continuous = NULL) {
+  if (is.discrete(limits)) {
+    n_discrete_limits <- length(limits)
+  } else {
+    n_discrete_limits <- 0
+  }
 
-  n_limits <- length(limits)
   is_empty <- is.null(limits) && is.null(range_continuous)
-  is_only_continuous <- n_limits == 0
+  is_only_continuous <- n_discrete_limits == 0
   is_only_discrete <- is.null(range_continuous)
 
   if (is_empty) {
     expand_limits_continuous_trans(c(0, 1), expand, coord_limits, trans)
   } else if (is_only_continuous) {
     expand_limits_continuous_trans(range_continuous, expand, coord_limits, trans)
   } else if (is_only_discrete) {
-    expand_limits_continuous_trans(c(1, n_limits), expand, coord_limits, trans)
+    expand_limits_continuous_trans(c(1, n_discrete_limits), expand, coord_limits, trans)
   } else {
     # continuous and discrete
-    limit_info_discrete <- expand_limits_continuous_trans(c(1, n_limits), expand, coord_limits, trans)
+    limit_info_discrete <- expand_limits_continuous_trans(c(1, n_discrete_limits), expand, coord_limits, trans)
 
     # don't expand continuous range if there is also a discrete range
     limit_info_continuous <- expand_limits_continuous_trans(

tests/testthat/test-scale-expansion.r

@@ -103,3 +103,14 @@ test_that("expand_limits_continuous_trans() works with inverted transformations"
   expect_identical(limit_info$continuous_range, c(0, 3))
   expect_identical(limit_info$continuous_range_coord, c(0, -3))
 })
+
+test_that("expand_limits_scale_discrete() begrudgingly handles numeric limits", {
+  expect_identical(
+    expand_limits_discrete(
+      -1:-16,
+      coord_limits = c(NA, NA),
+      range_continuous = c(-15, -2)
+    ),
+    c(-15, -2)
+  )
+})

tests/testthat/test-scales-breaks-labels.r

@@ -138,13 +138,17 @@ test_that("discrete scales with no data have no breaks or labels", {
   expect_equal(sc$get_limits(), c(0, 1))
 })
 
+test_that("passing continuous limits to a discrete scale generates a warning", {
+  expect_warning(scale_x_discrete(limits = 1:3), "Continuous limits supplied to discrete scale")
+})
+
 test_that("suppressing breaks, minor_breask, and labels works", {
   expect_equal(scale_x_continuous(breaks = NULL, limits = c(1, 3))$get_breaks(), NULL)
-  expect_equal(scale_x_discrete(breaks = NULL, limits = c(1, 3))$get_breaks(), NULL)
+  expect_equal(scale_x_discrete(breaks = NULL, limits = c("one", "three"))$get_breaks(), NULL)
   expect_equal(scale_x_continuous(minor_breaks = NULL, limits = c(1, 3))$get_breaks_minor(), NULL)
 
   expect_equal(scale_x_continuous(labels = NULL, limits = c(1, 3))$get_labels(), NULL)
-  expect_equal(scale_x_discrete(labels = NULL, limits = c(1, 3))$get_labels(), NULL)
+  expect_equal(scale_x_discrete(labels = NULL, limits = c("one", "three"))$get_labels(), NULL)
 
   # date, datetime
   lims <- as.Date(c("2000/1/1", "2000/2/1"))