Skip to content

Watch potential partial matches in Geom/Stat parameters #6428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions R/facet-.R
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@
return(vec_cbind(data %|W|% NULL, PANEL = integer(0)))
}

vars <- params$facet %||% c(params$rows, params$cols)
vars <- params$facets %||% c(params$rows, params$cols)

Check warning on line 895 in R/facet-.R

View check run for this annotation

Codecov / codecov/patch

R/facet-.R#L895

Added line #L895 was not covered by tests

if (length(vars) == 0) {
data$PANEL <- layout$PANEL
Expand All @@ -911,7 +911,7 @@
# Compute faceting values
facet_vals <- eval_facets(vars, data, params$.possible_columns)

include_margins <- !isFALSE(params$margin %||% FALSE) &&
include_margins <- !isFALSE(params$margins %||% FALSE) &&

Check warning on line 914 in R/facet-.R

View check run for this annotation

Codecov / codecov/patch

R/facet-.R#L914

Added line #L914 was not covered by tests
nrow(facet_vals) == nrow(data) && grid_layout
if (include_margins) {
# Margins are computed on evaluated faceting values (#1864).
Expand Down
2 changes: 1 addition & 1 deletion R/geom-smooth.R
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ geom_smooth <- function(mapping = NULL, data = NULL,
...
)
if (identical(stat, "smooth")) {
params$method <- method
params[["method"]] <- method
params$formula <- formula
}

Expand Down
2 changes: 1 addition & 1 deletion R/labels.R
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ get_alt_text.gtable <- function(p, ...) {
#'
generate_alt_text <- function(p) {
# Combine titles
if (!is.null(p$label$title %||% p$labels$subtitle)) {
if (!is.null(p$labels$title %||% p$labels$subtitle)) {
title <- sub("\\.?$", "", c(p$labels$title, p$labels$subtitle))
if (length(title) == 2) {
title <- paste0(title[1], ": ", title[2])
Expand Down
16 changes: 8 additions & 8 deletions R/legend-draw.R
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,11 @@
lty = data$linetype %||% 1,
lineend = params$lineend %||% "butt"
),
arrow = params$arrow
arrow = params[["arrow"]]
)
if (!is.null(params$arrow)) {
angle <- deg2rad(params$arrow$angle)
length <- convertUnit(params$arrow$length, "cm", valueOnly = TRUE)
if (!is.null(params[["arrow"]])) {
angle <- deg2rad(params[["arrow"]]$angle)
length <- convertUnit(params[["arrow"]]$length, "cm", valueOnly = TRUE)

Check warning on line 214 in R/legend-draw.R

View check run for this annotation

Codecov / codecov/patch

R/legend-draw.R#L213-L214

Added lines #L213 - L214 were not covered by tests
attr(grob, "width") <- cos(angle) * length * 1.25
attr(grob, "height") <- sin(angle) * length * 2
}
Expand All @@ -228,11 +228,11 @@
lty = data$linetype %||% 1,
lineend = params$lineend %||% "butt"
),
arrow = params$arrow
arrow = params[["arrow"]]
)
if (!is.null(params$arrow)) {
angle <- deg2rad(params$arrow$angle)
length <- convertUnit(params$arrow$length, "cm", valueOnly = TRUE)
if (!is.null(params[["arrow"]])) {
angle <- deg2rad(params[["arrow"]]$angle)
length <- convertUnit(params[["arrow"]]$length, "cm", valueOnly = TRUE)

Check warning on line 235 in R/legend-draw.R

View check run for this annotation

Codecov / codecov/patch

R/legend-draw.R#L234-L235

Added lines #L234 - L235 were not covered by tests
attr(grob, "width") <- sin(angle) * length * 2
attr(grob, "height") <- cos(angle) * length * 1.25
}
Expand Down
2 changes: 1 addition & 1 deletion R/stat-density-2d.R
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ StatDensity2d <- ggproto("StatDensity2d", Stat,
df$group <- data$group[1]
df$ndensity <- df$density / max(df$density, na.rm = TRUE)
df$count <- nx * df$density
df$n <- nx
df[["n"]] <- nx
df$level <- 1
df$piece <- 1
df
Expand Down
4 changes: 2 additions & 2 deletions R/stat-manual.R
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ StatManual <- ggproto(
"StatManual", Stat,

setup_params = function(data, params) {
params$fun <- allow_lambda(params$fun)
check_function(params$fun, arg = "fun")
params[["fun"]] <- allow_lambda(params[["fun"]])
check_function(params[["fun"]], arg = "fun")
params
},

Expand Down
6 changes: 3 additions & 3 deletions R/stat-smooth.R
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ StatSmooth <- ggproto("StatSmooth", Stat,
setup_params = function(data, params) {
params$flipped_aes <- has_flipped_aes(data, params, ambiguous = TRUE)
msg <- character()
method <- params$method
method <- params[["method"]]
if (is.null(method) || identical(method, "auto")) {
# Use loess for small datasets, gam with a cubic regression basis for
# larger. Based on size of the _largest_ group to avoid bad memory
Expand Down Expand Up @@ -144,14 +144,14 @@ StatSmooth <- ggproto("StatSmooth", Stat,
}
# If gam and gam's method is not specified by the user then use REML
if (identical(method, gam_method())) {
params$method.args$method <- params$method.args$method %||% "REML"
params$method.args[["method"]] <- params$method.args[["method"]] %||% "REML"
}

if (length(msg) > 0) {
cli::cli_inform("{.fn geom_smooth} using {msg}")
}

params$method <- method
params[["method"]] <- method
params
},

Expand Down
4 changes: 2 additions & 2 deletions R/stat-summary-bin.R
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ StatSummaryBin <- ggproto("StatSummaryBin", Stat,

setup_params = function(data, params) {
params$flipped_aes <- has_flipped_aes(data, params)
params$fun <- make_summary_fun(
params$fun.data, params$fun,
params[["fun"]] <- make_summary_fun(
params$fun.data, params[["fun"]],
params$fun.max, params$fun.min,
params$fun.args %||% list()
)
Expand Down
4 changes: 2 additions & 2 deletions R/stat-summary.R
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ StatSummary <- ggproto("StatSummary", Stat,

setup_params = function(data, params) {
params$flipped_aes <- has_flipped_aes(data, params)
params$fun <- make_summary_fun(
params$fun.data, params$fun,
params[["fun"]] <- make_summary_fun(
params$fun.data, params[["fun"]],
params$fun.max, params$fun.min,
params$fun.args %||% list()
)
Expand Down
4 changes: 2 additions & 2 deletions R/stat-ydensity.R
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ StatYdensity <- ggproto("StatYdensity", Stat,
trim = trim, na.rm = na.rm, drop = drop, bounds = bounds,
quantiles = quantiles
)
if (!drop && any(data$n < 2)) {
if (!drop && any(data[["n"]] < 2)) {
cli::cli_warn(
"Cannot compute density for groups with fewer than two datapoints."
)
Expand All @@ -184,7 +184,7 @@ StatYdensity <- ggproto("StatYdensity", Stat,
# count: use the original densities scaled to a maximum of 1 (as above)
# and then scale them according to the number of observations
count = data$density / max(data$density, na.rm = TRUE) *
data$n / max(data$n),
data[["n"]] / max(data[["n"]]),
# width: constant width (density scaled to a maximum of 1)
width = data$scaled
)
Expand Down
4 changes: 2 additions & 2 deletions R/theme-elements.R
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,10 @@ element_grob.element_line <- function(element, x = 0:1, y = 0:1,
linewidth <- size
}

arrow <- if (is.logical(element$arrow) && !element$arrow) {
arrow <- if (is.logical(element[["arrow"]]) && !element[["arrow"]]) {
NULL
} else {
element$arrow
element[["arrow"]]
}
if (is.null(arrow)) {
arrow.fill <- colour
Expand Down
31 changes: 31 additions & 0 deletions tests/testthat/_snaps/function-args.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# GeomXxx$parameters() does not contain partial matches

Code
problems
Output
[1] "GeomBoxplot : `notch` with `notchwidth`"
[2] "GeomContour : `arrow` with `arrow.fill`"
[3] "GeomCurve : `arrow` with `arrow.fill`"
[4] "GeomDensity2d: `arrow` with `arrow.fill`"
[5] "GeomFunction : `arrow` with `arrow.fill`"
[6] "GeomLine : `arrow` with `arrow.fill`"
[7] "GeomPath : `arrow` with `arrow.fill`"
[8] "GeomQuantile : `arrow` with `arrow.fill`"
[9] "GeomSegment : `arrow` with `arrow.fill`"
[10] "GeomSf : `arrow` with `arrow.fill`"
[11] "GeomSpoke : `arrow` with `arrow.fill`"
[12] "GeomStep : `arrow` with `arrow.fill`"

# StatXxx$parameters() does not contain partial matches

Code
problems
Output
[1] "StatDensity : `n` with `na.rm`"
[2] "StatDensity2d : `na.rm` with `n`"
[3] "StatDensity2dFilled: `na.rm` with `n`"
[4] "StatQuantile : `method` with `method.args`"
[5] "StatSmooth : `method` with `method.args`, `n` with `na.rm`"
[6] "StatSummary2d : `fun` with `fun.args`"
[7] "StatSummaryHex : `fun` with `fun.args`"

67 changes: 67 additions & 0 deletions tests/testthat/test-function-args.R
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,23 @@ filter_args <- function(x) {
x[all_names]
}

find_partial_match_pairs <- function(args) {
if (length(args) < 2) {
return(NULL)
}
combinations <- combn(args, 2L)
contains <- startsWith(combinations[1, ], combinations[2, ]) |
startsWith(combinations[2, ], combinations[1, ])

if (!any(contains)) {
return(NULL)
}

problem <- combinations[, contains, drop = FALSE]
paste0("`", problem[1, ], "` with `", problem[2, ], "`")
}


test_that("geom_xxx and GeomXxx$draw arg defaults match", {
ggplot2_ns <- asNamespace("ggplot2")
objs <- ls(ggplot2_ns)
Expand Down Expand Up @@ -73,3 +90,53 @@ test_that("stat_xxx and StatXxx$compute_panel arg defaults match", {
)
})
})

# If the following tests fail, you may have introduced a potential partial match
# in argument names. The code should be double checked that is doesn't
# accidentally use `list$arg` when `list$arg_name` also exists. If that doesn't
# occur, the snapshot can be updated.

test_that("GeomXxx$parameters() does not contain partial matches", {
ggplot2_ns <- asNamespace("ggplot2")
objs <- ls(ggplot2_ns)
geom_class_names <- grep("^Geom", objs, value = TRUE)
geom_class_names <- setdiff(geom_class_names, c("Geom"))

problems <- list()

for (geom_class_name in geom_class_names) {
geom_obj <- ggplot2_ns[[geom_class_name]]
params <- geom_obj$parameters()
issues <- find_partial_match_pairs(params)
if (length(issues) == 0) {
next
}
problems[[geom_class_name]] <- issues
}

problems <- vapply(problems, paste0, character(1), collapse = ", ")
problems <- paste0(format(names(problems)), ": ", problems)
expect_snapshot(problems)
})

test_that("StatXxx$parameters() does not contain partial matches", {
ggplot2_ns <- asNamespace("ggplot2")
objs <- ls(ggplot2_ns)
stat_class_names <- grep("^Stat", objs, value = TRUE)
stat_class_names <- setdiff(stat_class_names, c("Stat"))

problems <- list()

for (stat_class_name in stat_class_names) {
stat_obj <- ggplot2_ns[[stat_class_name]]
params <- stat_obj$parameters()
issues <- find_partial_match_pairs(params)
if (length(issues) == 0) {
next
}
problems[[stat_class_name]] <- issues
}
problems <- vapply(problems, paste0, character(1), collapse = ", ")
problems <- paste0(format(names(problems)), ": ", problems)
expect_snapshot(problems)
})
2 changes: 1 addition & 1 deletion tests/testthat/test-legend-draw.R
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ test_that("all keys can be drawn without 'params'", {
expect_in(nse, names(keys))

# Add title to every key
template <- gtable(width = unit(size, "mm"), heights = unit(c(1, size), c("lines", "mm")))
template <- gtable(widths = unit(size, "mm"), heights = unit(c(1, size), c("lines", "mm")))
keys <- Map(
function(key, name) {
text <- textGrob(name, gp = gpar(fontsize = 8))
Expand Down