Add type checking to default discrete scales (#4470)

Author: thomasp85

NEWS.md

@@ -1,5 +1,9 @@
 # ggplot2 (development version)
 
+* Remove cross-inheritance of default discrete colour/fill scales and check the
+  type and aesthetic of function output if `type` is a function 
+  (@thomasp85, #4149)
+
 * Add support for the BrailleR package for creating descriptions of the plot
   when rendered (@thomasp85, #4459)
 

R/scale-colour.r

@@ -160,15 +160,19 @@ scale_fill_binned <- function(...,
 
 # helper function to make sure that the provided scale is of the correct
 # type (i.e., is continuous and works with the provided aesthetic)
-check_scale_type <- function(scale, name, aesthetic) {
+check_scale_type <- function(scale, name, aesthetic, scale_is_discrete = FALSE) {
   if (!is.ggproto(scale) || !inherits(scale, "Scale")) {
     abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic. The provided object is not a scale function."))
   }
   if (!isTRUE(aesthetic %in% scale$aesthetics)) {
     abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic. The provided scale works with the following aesthetics: {glue_collapse(scale$aesthetics, sep = ', ')}"))
   }
-  if (isTRUE(scale$is_discrete())) {
-    abort(glue("The `type` argument of `{name}()` must return a continuous scale for the {aesthetic} aesthetic, but the provided scale is discrete."))
+  if (isTRUE(scale$is_discrete()) != scale_is_discrete) {
+    scale_types <- c("continuous", "discrete")
+    if (scale_is_discrete) {
+      scale_types <- rev(scale_types)
+    }
+    abort(glue("The `type` argument of `{name}()` must return a {scale_types[1]} scale for the {aesthetic} aesthetic, but the provided scale is {scale_types[2]}."))
   }
 
   scale

R/scale-hue.r

@@ -118,23 +118,33 @@ scale_fill_hue <- function(..., h = c(0, 360) + 15, c = 100, l = 65, h.start = 0
 #'   print(cty_by_var(fl))
 #' })
 #'
-scale_colour_discrete <- function(..., type = getOption("ggplot2.discrete.colour", getOption("ggplot2.discrete.fill"))) {
+scale_colour_discrete <- function(..., type = getOption("ggplot2.discrete.colour")) {
   # TODO: eventually `type` should default to a set of colour-blind safe color codes (e.g. Okabe-Ito)
   type <- type %||% scale_colour_hue
   if (is.function(type)) {
-    type(...)
+    check_scale_type(
+      type(...),
+      "scale_colour_discrete",
+      "colour",
+      scale_is_discrete = TRUE
+    )
   } else {
     scale_colour_qualitative(..., type = type)
   }
 }
 
 #' @rdname scale_colour_discrete
 #' @export
-scale_fill_discrete <- function(..., type = getOption("ggplot2.discrete.fill", getOption("ggplot2.discrete.colour"))) {
+scale_fill_discrete <- function(..., type = getOption("ggplot2.discrete.fill")) {
   # TODO: eventually `type` should default to a set of colour-blind safe color codes (e.g. Okabe-Ito)
   type <- type %||% scale_fill_hue
   if (is.function(type)) {
-    type(...)
+    check_scale_type(
+      type(...),
+      "scale_fill_discrete",
+      "fill",
+      scale_is_discrete = TRUE
+    )
   } else {
     scale_fill_qualitative(..., type = type)
   }

man/scale_colour_discrete.Rd

@@ -96,7 +96,8 @@ test_that("discrete scale defaults can be set globally", {
   )
 
   withr::with_options(
-    list(ggplot2.discrete.fill = c("#FFFFFF", "#000000")), {
+    list(ggplot2.discrete.fill = c("#FFFFFF", "#000000"),
+         ggplot2.discrete.colour = c("#FFFFFF", "#000000")), {
       # nlevels == ncodes
       two <- ggplot(df, aes(x, y, colour = two, fill = two)) + geom_point()
       expect_equal(layer_data(two)$colour, rep(c("#FFFFFF", "#000000"), 2))
@@ -107,13 +108,18 @@ test_that("discrete scale defaults can be set globally", {
         geom_point()
       four_hue <- four_default + scale_fill_hue()
       expect_equal(layer_data(four_default)$colour, layer_data(four_hue)$colour)
-  })
+    }
+  )
 
   withr::with_options(
     list(
       ggplot2.discrete.fill = list(
         c("#FFFFFF", "#000000"),
         c("#FF0000", "#00FF00", "#0000FF", "#FF00FF")
+      ),
+      ggplot2.discrete.colour = list(
+        c("#FFFFFF", "#000000"),
+        c("#FF0000", "#00FF00", "#0000FF", "#FF00FF")
       )
     ), {
       # nlevels == 2
@@ -125,7 +131,18 @@ test_that("discrete scale defaults can be set globally", {
       four <- ggplot(df, aes(x, y, colour = four, fill = four)) + geom_point()
       expect_equal(layer_data(four)$colour, c("#FF0000", "#00FF00", "#0000FF", "#FF00FF"))
       expect_equal(layer_data(four)$fill, c("#FF0000", "#00FF00", "#0000FF", "#FF00FF"))
-    })
+    }
+  )
+})
+
+test_that("Scale is checked in default colour scale", {
+  # Check scale type
+  expect_error(scale_colour_discrete(type = scale_colour_gradient))
+  expect_error(scale_fill_discrete(type = scale_fill_gradient))
+
+  # Check aesthetic
+  expect_error(scale_colour_discrete(type = scale_fill_hue))
+  expect_error(scale_fill_discrete(type = scale_colour_hue))
 })
 
 # mapped_discrete ---------------------------------------------------------