diff --git a/NEWS.md b/NEWS.md index 88abe25fe..301facb71 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,10 @@ * Arguments `allow_cascading_assign=`, `allow_right_assign=`, and `allow_pipe_assign=` to `assignment_linter()` are now removed. Use `operator=` instead. * Argument `interpret_glue` to `object_usage_linter()`, marked deprecated in the previous release, is now defunct. Use `interpret_extensions=` instead; see the 3.3.0-1 release notes and `?object_usage_linter` for more. +## Bug fixes + +* Excluding `cyclocomp_linter()` in `available_linters()` or `linters_with_tags()`, which requires the weak dependency {cyclocomp}, no longer emits a warning (#2909, @MichaelChirico). + ## Notes * {lintr} now requires R 4.1.0 diff --git a/R/with.R b/R/with.R index 56d2d6edd..8abbed4f8 100644 --- a/R/with.R +++ b/R/with.R @@ -34,8 +34,8 @@ modify_defaults <- function(defaults, ...) { if (missing(defaults)) { cli_abort("{.arg defaults} is a required argument, but is missing.") } - if (!is.list(defaults) || !all(nzchar(names2(defaults)))) { - cli_abort("{.arg defaults} must be a named list, not {.obj_type_friendly {defaults}}.") + if (!(is.list(defaults) || is.environment(defaults)) || !all(nzchar(names2(defaults)))) { + cli_abort("{.arg defaults} must be a named list or environment, not {.obj_type_friendly {defaults}}.") } vals <- list(...) nms <- names2(vals) @@ -53,7 +53,8 @@ modify_defaults <- function(defaults, ...) { } is.na(vals) <- nms == vals - defaults[nms] <- vals + for (ii in seq_along(nms)) defaults[[nms[ii]]] <- vals[[ii]] + if (is.environment(defaults)) defaults <- as.list(defaults) res <- defaults[!vapply(defaults, is.null, logical(1L))] res <- res[platform_independent_order(names(res))] @@ -98,11 +99,10 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep if (!is.character(tags) && !is.null(tags)) { cli_abort("{.arg tags} must be a character vector, or {.code NULL}, not {.obj_type_friendly {tags}}.") } - tagged_linters <- list() + tagged_linter_env <- new.env() for (package in packages) { - pkg_ns <- loadNamespace(package) - ns_exports <- getNamespaceExports(pkg_ns) + ns_exports <- getNamespaceExports(package) available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { @@ -112,18 +112,26 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } - linter_factories <- mget(available$linter, envir = pkg_ns) - linters <- Map( - call_linter_factory, - linter_factory = linter_factories, - linter_name = names(linter_factories), - MoreArgs = list(package = package) - ) - tagged_linters <- c(tagged_linters, linters) + for (linter in available$linter) lazily_assign_linter_(linter, package, tagged_linter_env) } } - modify_defaults(..., defaults = tagged_linters) + modify_defaults(..., defaults = tagged_linter_env) +} + +#' Avoid call_linter_factory up-front to delay displaying warnings until needed. +#' This e.g. allows modify_defaults() to skip warnings from linters that aren't needed. +#' NB: this helper has the very subtle effect of ensuring 'linter' is associated correctly; +#' an earlier attempt had this logic directly in a loop in linters_with_tags, but +#' that results in the value of 'linter' matching that of the loop index after the loop +#' completes, rather than what its value was when 'delayedAssign()' is called. local({}) +#' _can_ work, but requires the befuddling line 'linter <- linter' to ensure that the +#' local() environment retains a copy of that variable; the formals of a helper +#' have the same effect. +#' @noRd +lazily_assign_linter_ <- function(linter, package, env) { + linter_factory <- get(linter, envir = getNamespace(package), inherits = FALSE) + delayedAssign(linter, call_linter_factory(linter_factory, linter, package), assign.env = env) } #' Create a linter configuration based on all available linters diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 2dd9fb854..822c4df0b 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -117,3 +117,20 @@ test_that("all_linters respects ellipsis argument", { all_linters(packages = "lintr", implicit_integer_linter = NULL) ) }) + +test_that("Excluding cyclocomp linter avoids a warning", { + local_mocked_bindings( + requireNamespace = function(pkg, ...) pkg != "cyclocomp" || base::requireNamespace(pkg, ...) + ) + + expect_silent(all_linters(cyclocomp_linter = NULL)) + expect_silent(linters_with_tags("configurable", cyclocomp_linter = NULL)) +}) + +test_that("cyclocomp_linter does warn as intended", { + local_mocked_bindings( + requireNamespace = function(pkg, ...) pkg != "cyclocomp" && base::requireNamespace(pkg, ...) + ) + + expect_warning(linters_with_tags("configurable"), "cyclocomp::cyclocomp") +})