From 5135b563dffdc7859ecad5ec729637591d168e7f Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 17 Nov 2025 12:11:59 +0100 Subject: [PATCH 1/5] `find_global()` iterates over supplied environments --- R/scale-type.R | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/R/scale-type.R b/R/scale-type.R index e9f3b8cc9b..554e1a83ea 100644 --- a/R/scale-type.R +++ b/R/scale-type.R @@ -29,13 +29,16 @@ find_scale <- function(aes, x, env = parent.frame()) { # ggplot2 namespace environment. This makes it possible to override default # scales by setting them in the parent environment. find_global <- function(name, env, mode = "any") { - if (exists(name, envir = env, mode = mode)) { - return(get(name, envir = env, mode = mode)) + + if (!is.list(env)) { + env <- list(env) } + env <- c(env, list(asNamespace("ggplot2"))) - nsenv <- asNamespace("ggplot2") - if (exists(name, envir = nsenv, mode = mode)) { - return(get(name, envir = nsenv, mode = mode)) + for (e in env) { + if (exists(name, envir = e, mode = mode)) { + return(get(name, envir = e, mode = mode)) + } } NULL From 54f5ecf23d8429590acf4fbe21638983bfd1bac7 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 17 Nov 2025 12:29:59 +0100 Subject: [PATCH 2/5] expand search env when type has namespace prefix --- R/scale-type.R | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/R/scale-type.R b/R/scale-type.R index 554e1a83ea..61ea25b63a 100644 --- a/R/scale-type.R +++ b/R/scale-type.R @@ -10,7 +10,15 @@ find_scale <- function(aes, x, env = parent.frame()) { candidates <- paste("scale", aes, type, sep = "_") for (scale in candidates) { - scale_f <- find_global(scale, env, mode = "function") + search_env <- list(env) + if (isTRUE(grepl("::", scale))) { + # Append prefix as namepaces to search environments + prefix <- sub("::.*", "", scale) + search_env <- c(search_env, list(asNamespace(prefix))) + # Remove prefix from scale name + scale <- sub(".*::", "", guide) + } + scale_f <- find_global(scale, search_env, mode = "function") if (!is.null(scale_f)) { sc <- scale_f() sc$call <- parse_expr(paste0(scale, "()")) From 2ccaf4044605261a8b4af01802ce8c8c838525e6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 17 Nov 2025 12:30:07 +0100 Subject: [PATCH 3/5] same for guides --- R/guides-.R | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/R/guides-.R b/R/guides-.R index fa50f5a79d..476849921c 100644 --- a/R/guides-.R +++ b/R/guides-.R @@ -915,8 +915,19 @@ include_layer_in_guide <- function(layer, matched) { validate_guide <- function(guide) { # if guide is specified by character, then find the corresponding guide if (is.character(guide)) { - fun <- find_global(paste0("guide_", guide), env = global_env(), - mode = "function") + check_string(guide, allow_empty = FALSE) + search_env <- list(global_env()) + if (isTRUE(grepl("::", guide))) { + # Append prefix as namespaces to search environments + prefix <- sub("::.*", "", guide) + search_env <- c(search_env, list(asNamespace(prefix))) + # Remove prefix from guide name + guide <- sub(".*::", "", guide) + } + fun <- find_global( + paste0("guide_", guide), + env = search_env, mode = "function" + ) if (is.function(fun)) { guide <- fun() } From 060b7cd4a5e57d4044188e921da4455179e39e6b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 17 Nov 2025 13:33:07 +0100 Subject: [PATCH 4/5] fix bug in `find_scale()` --- R/scale-type.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/scale-type.R b/R/scale-type.R index 61ea25b63a..1c5b0f7081 100644 --- a/R/scale-type.R +++ b/R/scale-type.R @@ -7,17 +7,17 @@ find_scale <- function(aes, x, env = parent.frame()) { } type <- scale_type(x) - candidates <- paste("scale", aes, type, sep = "_") - for (scale in candidates) { + for (scale in type) { search_env <- list(env) if (isTRUE(grepl("::", scale))) { # Append prefix as namepaces to search environments prefix <- sub("::.*", "", scale) search_env <- c(search_env, list(asNamespace(prefix))) # Remove prefix from scale name - scale <- sub(".*::", "", guide) + scale <- sub(".*::", "", scale) } + scale <- paste("scale", aes, scale, sep = "_") scale_f <- find_global(scale, search_env, mode = "function") if (!is.null(scale_f)) { sc <- scale_f() From 65930487bcf45dd4be42078494109797a463b46d Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 18 Nov 2025 13:25:22 +0100 Subject: [PATCH 5/5] test by mocking --- R/guides-.R | 2 +- R/scale-type.R | 11 +++++++-- tests/testthat/_snaps/guides.md | 4 ++++ tests/testthat/test-guides.R | 28 +++++++++++++++++++++++ tests/testthat/test-scale-type.R | 38 ++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 3 deletions(-) diff --git a/R/guides-.R b/R/guides-.R index 476849921c..711c7bc225 100644 --- a/R/guides-.R +++ b/R/guides-.R @@ -920,7 +920,7 @@ validate_guide <- function(guide) { if (isTRUE(grepl("::", guide))) { # Append prefix as namespaces to search environments prefix <- sub("::.*", "", guide) - search_env <- c(search_env, list(asNamespace(prefix))) + search_env <- c(search_env, list(as_namespace(prefix))) # Remove prefix from guide name guide <- sub(".*::", "", guide) } diff --git a/R/scale-type.R b/R/scale-type.R index 1c5b0f7081..e20a9b8eb8 100644 --- a/R/scale-type.R +++ b/R/scale-type.R @@ -13,7 +13,7 @@ find_scale <- function(aes, x, env = parent.frame()) { if (isTRUE(grepl("::", scale))) { # Append prefix as namepaces to search environments prefix <- sub("::.*", "", scale) - search_env <- c(search_env, list(asNamespace(prefix))) + search_env <- c(search_env, list(as_namespace(prefix))) # Remove prefix from scale name scale <- sub(".*::", "", scale) } @@ -41,7 +41,7 @@ find_global <- function(name, env, mode = "any") { if (!is.list(env)) { env <- list(env) } - env <- c(env, list(asNamespace("ggplot2"))) + env <- c(env, list(as_namespace("ggplot2"))) for (e in env) { if (exists(name, envir = e, mode = mode)) { @@ -52,6 +52,13 @@ find_global <- function(name, env, mode = "any") { NULL } +# This exists for testing purposes (mocking) only +as_namespace <- function(...) NULL +on_load({ + as_namespace <- base::asNamespace +}) + + #' Determine default scale type #' #' You will need to define a method for this method if you want to extend diff --git a/tests/testthat/_snaps/guides.md b/tests/testthat/_snaps/guides.md index a47fba746b..e7c02b78e0 100644 --- a/tests/testthat/_snaps/guides.md +++ b/tests/testthat/_snaps/guides.md @@ -37,6 +37,10 @@ `nrow` * `ncol` needs to be larger than the number of breaks (5). +# validate_guide finds guides with namespace prefixes + + Unknown guide: bar + # get_guide_data retrieves keys appropriately Code diff --git a/tests/testthat/test-guides.R b/tests/testthat/test-guides.R index ae1bfe85bd..a75a46ebb1 100644 --- a/tests/testthat/test-guides.R +++ b/tests/testthat/test-guides.R @@ -51,6 +51,34 @@ test_that("guide specifications are properly checked", { expect_snapshot_error(ggplotGrob(p)) }) + +test_that("validate_guide finds guides with namespace prefixes", { + + # Mock foo::bar as namespace + fake_namespace <- new_environment() + env_bind( + fake_namespace, + guide_bar = function(...) guide_legend(title = "bar", ...) + ) + + local_mocked_bindings( + as_namespace = function(ns, ...) { + if (identical(ns, "foo")) { + return(fake_namespace) + } else { + base::asNamespace(ns, ...) + } + } + ) + + # Without prefix, we don't know here to look for guide_bar + expect_snapshot_error(validate_guide("bar")) + # With prefix, we know the namespace where to look for guide_bar + g <- validate_guide("foo::bar") + expect_true(is_guide(g)) + expect_equal(g$params$title, "bar") +}) + test_that("guide_coloursteps and guide_bins return ordered breaks", { scale <- scale_colour_viridis_c(breaks = c(2, 3, 1)) scale$train(c(0, 4)) diff --git a/tests/testthat/test-scale-type.R b/tests/testthat/test-scale-type.R index 3ca1f06637..c894a51e82 100644 --- a/tests/testthat/test-scale-type.R +++ b/tests/testthat/test-scale-type.R @@ -24,3 +24,41 @@ test_that("find_scale gives sensible calls to scales", { quote(scale_colour_discrete()) ) }) + +test_that("find_scale finds scales with namespace prefixes", { + + # Mock foo::bar as namespace + fake_namespace <- new_environment() + env_bind( + fake_namespace, + scale_x_bar = function(...) scale_x_continuous(name = "barname") + ) + + local_mocked_bindings( + as_namespace = function(ns, ...) { + if (identical(ns, "foo")) { + return(fake_namespace) + } else { + base::asNamespace(ns, ...) + } + } + ) + + # No loaded namespace has a scale_x_bar + registerS3method( + "scale_type", "bar", + method = function(x) "bar" + ) + + sc <- find_scale("x", structure(1, class = "bar")) + expect_null(sc) + + # With prefix, we know the namespace where to look for scale_x_bar + registerS3method( + "scale_type", "bar", + method = function(x) "foo::bar" + ) + + sc <- find_scale("x", structure(1, class = "bar")) + expect_equal(sc$name, "barname") +})