Merge pull request #3286 from paleolimbot/issue-3090-warn-data-hline-vline-abline

redo warnings for overwritten arguments in abline, hline, and vline (fixes #3090)

Author: paleolimbot

NEWS.md

@@ -1,5 +1,10 @@
 # ggplot2 (development version)
 
+* `geom_abline()`, `geom_hline()`, and `geom_vline()` now issue 
+  more informative warnings when supplied with set aesthetics
+  (i.e., `slope`, `intercept`, `yintercept`, and/or `xintercept`)
+  and mapped aesthetics (i.e., `data` and/or `mapping`).
+
 # ggplot2 3.1.1.9000
 
 This is a minor release with an emphasis on internal changes to make ggplot2 

R/geom-abline.r

@@ -76,21 +76,20 @@ geom_abline <- function(mapping = NULL, data = NULL,
                         show.legend = NA) {
 
   # If nothing set, default to y = x
-  if (missing(mapping) && missing(slope) && missing(intercept)) {
+  if (is.null(mapping) && missing(slope) && missing(intercept)) {
     slope <- 1
     intercept <- 0
   }
 
   # Act like an annotation
   if (!missing(slope) || !missing(intercept)) {
 
-    # Warn if supplied mapping is going to be overwritten
-    if (!missing(mapping)) {
-      warning(paste0("Using `intercept` and/or `slope` with `mapping` may",
-                     " not have the desired result as mapping is overwritten",
-                     " if either of these is specified\n"
-              )
-      )
+    # Warn if supplied mapping and/or data is going to be overwritten
+    if (!is.null(mapping)) {
+      warn_overwritten_args("geom_abline()", "mapping", c("slope", "intercept"))
+    }
+    if (!is.null(data)) {
+      warn_overwritten_args("geom_abline()", "data", c("slope", "intercept"))
     }
 
     if (missing(slope)) slope <- 1
@@ -141,3 +140,34 @@ GeomAbline <- ggproto("GeomAbline", Geom,
 
   draw_key = draw_key_abline
 )
+
+warn_overwritten_args <- function(fun_name, overwritten_arg, provided_args, plural_join = " and/or ") {
+  overwritten_arg_text <- paste0("`", overwritten_arg, "`")
+
+  n_provided_args <- length(provided_args)
+  if (n_provided_args == 1) {
+    provided_arg_text <- paste0("`", provided_args, "`")
+    verb <- "was"
+  } else if (n_provided_args == 2) {
+    provided_arg_text <- paste0("`", provided_args, "`", collapse = plural_join)
+    verb <- "were"
+  } else {
+    provided_arg_text <- paste0(
+      paste0("`", provided_args[-n_provided_args], "`", collapse = ", "),
+      ",", plural_join,
+      "`", provided_args[n_provided_args], "`"
+    )
+    verb <- "were"
+  }
+
+  warning(
+    sprintf(
+      "%s: Ignoring %s because %s %s provided.",
+      fun_name,
+      overwritten_arg_text,
+      provided_arg_text,
+      verb
+    ),
+    call. = FALSE
+  )
+}

R/geom-hline.r

@@ -11,14 +11,14 @@ geom_hline <- function(mapping = NULL, data = NULL,
 
   # Act like an annotation
   if (!missing(yintercept)) {
-    # Warn if supplied mapping is going to be overwritten
-    if (!missing(mapping)) {
-      warning(paste0("Using both `yintercept` and `mapping` may not have the",
-                     " desired result as mapping is overwritten if",
-                     " `yintercept` is specified\n"
-              )
-      )
+    # Warn if supplied mapping and/or data is going to be overwritten
+    if (!is.null(mapping)) {
+      warn_overwritten_args("geom_hline()", "mapping", "yintercept")
     }
+    if (!is.null(data)) {
+      warn_overwritten_args("geom_hline()", "data", "yintercept")
+    }
+
     data <- new_data_frame(list(yintercept = yintercept))
     mapping <- aes(yintercept = yintercept)
     show.legend <- FALSE

R/geom-vline.r

@@ -11,14 +11,14 @@ geom_vline <- function(mapping = NULL, data = NULL,
 
   # Act like an annotation
   if (!missing(xintercept)) {
-    # Warn if supplied mapping is going to be overwritten
-    if (!missing(mapping)) {
-      warning(paste0("Using both `xintercept` and `mapping` may not have the",
-                     " desired result as mapping is overwritten if",
-                     " `xintercept` is specified\n"
-              )
-      )
+    # Warn if supplied mapping and/or data is going to be overwritten
+    if (!is.null(mapping)) {
+      warn_overwritten_args("geom_vline()", "mapping", "xintercept")
     }
+    if (!is.null(data)) {
+      warn_overwritten_args("geom_vline()", "data", "xintercept")
+    }
+
     data <- new_data_frame(list(xintercept = xintercept))
     mapping <- aes(xintercept = xintercept)
     show.legend <- FALSE

tests/testthat/test-geom-hline-vline-abline.R

@@ -46,30 +46,76 @@ test_that("curved lines in map projections", {
 
 # Warning tests ------------------------------------------------------------
 
+test_that("warn_overwritten_args() produces gramatically correct error messages", {
+  expect_warning(
+    warn_overwritten_args("fun_test", "is_overwritten", "provided"),
+    "fun_test: Ignoring `is_overwritten` because `provided` was provided."
+  )
+  expect_warning(
+    warn_overwritten_args("fun_test", "is_overwritten", c("provided1", "provided2")),
+    "fun_test: Ignoring `is_overwritten` because `provided1` and/or `provided2` were provided."
+  )
+  expect_warning(
+    warn_overwritten_args("fun_test", "is_overwritten", c("provided1", "provided2", "provided3")),
+    "fun_test: Ignoring `is_overwritten` because `provided1`, `provided2`, and/or `provided3` were provided."
+  )
+})
+
 test_that("Warning if a supplied mapping is going to be overwritten", {
 
   expect_warning(
     geom_vline(xintercept = 3, aes(colour = colour)),
-    "Using both"
+    "Ignoring `mapping`"
   )
 
   expect_warning(
     geom_hline(yintercept = 3, aes(colour = colour)),
-    "Using both"
+    "Ignoring `mapping`"
   )
 
   expect_warning(
     geom_abline(intercept = 3, aes(colour = colour)),
-    "Using "
+    "Ignoring `mapping`"
   )
 
   expect_warning(
     geom_abline(intercept = 3, slope = 0.5, aes(colour = colour)),
-    "Using "
+    "Ignoring `mapping`"
+  )
+
+  expect_warning(
+    geom_abline(slope = 0.5, aes(colour = colour)),
+    "Ignoring `mapping`"
+  )
+})
+
+
+test_that("Warning if supplied data is going to be overwritten", {
+
+  sample_data <- data_frame(x = 1)
+
+  expect_warning(
+    geom_vline(xintercept = 3, data = sample_data),
+    "Ignoring `data`"
+  )
+
+  expect_warning(
+    geom_hline(yintercept = 3, data = sample_data),
+    "Ignoring `data`"
+  )
+
+  expect_warning(
+    geom_abline(intercept = 3, data = sample_data),
+    "Ignoring `data`"
+  )
+
+  expect_warning(
+    geom_abline(intercept = 3, slope = 0.5, data = sample_data),
+    "Ignoring `data`"
   )
 
   expect_warning(
-    geom_abline(slope=0.5, aes(colour = colour)),
-    "Using "
+    geom_abline(slope = 0.5, data = sample_data),
+    "Ignoring `data`"
   )
 })