Skip to content

Commit

Permalink
Check ellipses in several places (#6031)
Browse files Browse the repository at this point in the history
* helper function to throw error demoted to warning

* check `fortify()` dots

* check `get_alt_text()` dots

* check `deprecated_guide_args()` dots

* check for unknown labels

* add news bullet
  • Loading branch information
teunbrand committed Aug 28, 2024
1 parent 2e08bba commit a49bf1e
Show file tree
Hide file tree
Showing 13 changed files with 95 additions and 8 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@
* `theme_classic()` now has black ticks and text instead of dark gray. In
addition, `theme_classic()`'s axis line end is `"square"` (@teunbrand, #5978).
* {tibble} is now suggested instead of imported (@teunbrand, #5986)
* The ellipsis argument is now checked in `fortify()`, `get_alt_text()`,
`labs()` and several guides (@teunbrand, #3196).

# ggplot2 3.5.1

Expand Down
7 changes: 5 additions & 2 deletions R/fortify.R
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@
#' @seealso [fortify.lm()]
#' @param model model or other R object to convert to data frame
#' @param data original dataset, if needed
#' @param ... other arguments passed to methods
#' @inheritParams rlang::args_dots_used
#' @export
fortify <- function(model, data, ...) UseMethod("fortify")
fortify <- function(model, data, ...) {
warn_dots_used()
UseMethod("fortify")
}

#' @export
fortify.data.frame <- function(model, data, ...) model
Expand Down
1 change: 1 addition & 0 deletions R/guide-legend.R
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ deprecated_guide_args <- function(
default.unit = "line",
...,
.call = caller_call()) {
warn_dots_used(call = .call)

args <- names(formals(deprecated_guide_args))
args <- setdiff(args, c("theme", "default.unit", "...", ".call"))
Expand Down
28 changes: 24 additions & 4 deletions R/labels.R
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ update_labels <- function(p, labels) {

# Called in `ggplot_build()` to set default labels not specified by user.
setup_plot_labels <- function(plot, layers, data) {
# Initiate from user-defined labels
labels <- plot$labels
# Initiate empty labels
labels <- list()

# Find labels from every layer
for (i in seq_along(layers)) {
Expand Down Expand Up @@ -65,7 +65,26 @@ setup_plot_labels <- function(plot, layers, data) {
labels <- defaults(labels, current)
}
}
labels

# Warn for spurious labels that don't have a mapping.
# Note: sometimes, 'x' and 'y' might not have a mapping, like in
# `geom_function()`. We can display these labels anyway, so we include them.
plot_labels <- plot$labels
known_labels <- c(names(labels), fn_fmls_names(labs), "x", "y")
extra_labels <- setdiff(names(plot_labels), known_labels)

if (length(extra_labels) > 0) {
extra_labels <- paste0(
"{.code ", extra_labels, " = \"", plot_labels[extra_labels], "\"}"
)
names(extra_labels) <- rep("*", length(extra_labels))
cli::cli_warn(c(
"Ignoring unknown labels:",
extra_labels
))
}

defaults(plot_labels, labels)
}

#' Modify axis, legend, and plot labels
Expand Down Expand Up @@ -168,7 +187,7 @@ ggtitle <- function(label, subtitle = waiver()) {
#' text from the information stored in the plot.
#'
#' @param p a ggplot object
#' @param ... Currently ignored
#' @inheritParams rlang::args_dots_used
#'
#' @return A text string
#'
Expand All @@ -191,6 +210,7 @@ ggtitle <- function(label, subtitle = waiver()) {
#' get_alt_text(p)
#'
get_alt_text <- function(p, ...) {
warn_dots_used()
UseMethod("get_alt_text")
}
#' @export
Expand Down
12 changes: 12 additions & 0 deletions R/utilities.R
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,18 @@ as_unordered_factor <- function(x) {
x
}

warn_dots_used <- function(env = caller_env(), call = caller_env()) {
check_dots_used(
env = env, call = call,
# Demote from error to warning
error = function(cnd) {
# cli uses \f as newlines, not \n
msg <- gsub("\n", "\f", cnd_message(cnd))
cli::cli_warn(msg, call = call)
}
)
}

# Shim for scales/#424
col_mix <- function(a, b, amount = 0.5) {
input <- vec_recycle_common(a = a, b = b, amount = amount)
Expand Down
2 changes: 1 addition & 1 deletion man/fortify.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion man/get_alt_text.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions tests/testthat/_snaps/guides.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
# dots are checked when making guides

Ignoring unknown argument to `guide_axis()`: `foo`.

---

Arguments in `...` must be used.
x Problematic argument:
* foo = "bar"
i Did you misspell an argument name?

# Using non-position guides for position scales results in an informative error

`guide_legend()` cannot be used for x, xmin, xmax, or xend.
Expand Down
12 changes: 12 additions & 0 deletions tests/testthat/_snaps/labels.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@
Output
[1] "A plot showing class on a discrete x-axis and count on a continuous y-axis using a bar layer."

# get_alt_text checks dots

Arguments in `...` must be used.
x Problematic argument:
* foo = "bar"
i Did you misspell an argument name?

# warnings are thrown for unknown labels

Ignoring unknown labels:
* `foo = "bar"`

# plot.tag.position rejects invalid input

The `plot.tag.position` theme element must be a <character/numeric/integer> object.
Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/_snaps/plot.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
`data` cannot be a function.
i Have you misspelled the `data` argument in `ggplot()`

---

Arguments in `...` must be used.
x Problematic argument:
* foobar = "nonsense"
i Did you misspell an argument name?

# construction have user friendly errors

Cannot use `+` with a single argument.
Expand Down
9 changes: 9 additions & 0 deletions tests/testthat/test-guides.R
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@ test_that("show.legend handles named vectors", {
expect_equal(n_legends(p), 1)
})

test_that("dots are checked when making guides", {
expect_snapshot_warning(
new_guide(foo = "bar", super = GuideAxis)
)
expect_snapshot_warning(
guide_legend(foo = "bar")
)
})

test_that("axis_label_overlap_priority always returns the correct number of elements", {
expect_identical(axis_label_priority(0), numeric(0))
expect_setequal(axis_label_priority(1), seq_len(1))
Expand Down
9 changes: 9 additions & 0 deletions tests/testthat/test-labels.R
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,15 @@ test_that("alt text can take a function", {
expect_snapshot(get_alt_text(p))
})

test_that("get_alt_text checks dots", {
expect_snapshot_warning(get_alt_text(ggplot(), foo = "bar"))
})

test_that("warnings are thrown for unknown labels", {
p <- ggplot(mtcars, aes(mpg, disp)) + geom_point() + labs(foo = 'bar')
expect_snapshot_warning(ggplot_build(p))
})

test_that("plot.tag.position rejects invalid input", {
p <- ggplot(mtcars, aes(mpg, disp)) + geom_point() + labs(tag = "Fig. A)")

Expand Down
1 change: 1 addition & 0 deletions tests/testthat/test-plot.R
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
test_that("ggplot() throws informative errors", {
expect_snapshot_error(ggplot(mapping = letters))
expect_snapshot_error(ggplot(data))
expect_snapshot_warning(ggplot(foobar = "nonsense"))
})

test_that("construction have user friendly errors", {
Expand Down

0 comments on commit a49bf1e

Please sign in to comment.