From 3c1f87493eaead5196e805ee2724735f45da0a0f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 31 May 2022 19:12:55 -0700 Subject: [PATCH 1/4] add a test --- tests/testthat/test-with.R | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 94743e9f81..9dda359abf 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -61,3 +61,8 @@ test_that("modify_defaults works", { # auto-sorts expect_equal(modify_defaults(defaults = list(b = 2L, a = 1L), c = 3L), my_default) }) + +test_that("linters_with_defaults(default = .) is supported with a deprecation warning", { + expect_warning(linters <- linters_with_defaults(default = list(), no_tab_linter())) + expect_named(linters, "no_tab_linter") +}) From 18085b4e491ef6e5f4324a477f86aaf157fe94d9 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 31 May 2022 19:47:24 -0700 Subject: [PATCH 2/4] handle default= in modify_defaults for back-compatibility --- NEWS.md | 4 +++- R/with.R | 14 +++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index e51b374ab9..9bb03589c4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -52,7 +52,9 @@ It has also been renamed as the argument of the now-private functional versions of many linters, which has no direct effect on packages importing lintr, but is mentioned in case custom linters imitating `lintr` style have also adopted the `source_file` naming and want to adapt to keep in sync. -* Deprecated `with_defaults()` in favor of `linters_with_defaults()` (#1029, @AshesITR) +* Deprecated `with_defaults()` in favor of `linters_with_defaults()`, and add `modify_defaults()` which is intended to be used + more generally to modify (i.e., extend, trim, and/or update) a list of defaults. Note that the argument corresponding to + `with_defaults()`'s `default=` is called `defaults=` (i.e., pluralized) in both of these (#1029, #1336, @AshesITR and @michaelchirico) ## Other changes to defaults diff --git a/R/with.R b/R/with.R index ed3b21679c..ad0dc36664 100644 --- a/R/with.R +++ b/R/with.R @@ -24,9 +24,21 @@ modify_defaults <- function(defaults, ...) { } vals <- list(...) nms <- names2(vals) + if ("default" %in% nms) { + warning( + "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", + "This warning will be removed when with_defaults() is fully deprecated." + ) + defaults <- vals$default + vals$default <- NULL + valid_idx <- nms != "default" + nms <- nms[valid_idx] + } else { + valid_idx <- TRUE + } missing <- !nzchar(nms, keepNA = TRUE) if (any(missing)) { - args <- as.character(eval(substitute(alist(...)[missing]))) + args <- as.character(eval(substitute(alist(...)[missing])))[valid_idx] # foo_linter(x=1) => "foo" # var[["foo"]] => "foo" nms[missing] <- re_substitutes( From 86b612aa7d1fb7596777e103f79737ae152b4944 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 31 May 2022 21:10:31 -0700 Subject: [PATCH 3/4] tighten logic to only apply to linters_with_defaults --- R/with.R | 29 +++++++++++++++++++++-------- tests/testthat/test-with.R | 7 ++++++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/R/with.R b/R/with.R index ad0dc36664..7072c40f22 100644 --- a/R/with.R +++ b/R/with.R @@ -25,14 +25,27 @@ modify_defaults <- function(defaults, ...) { vals <- list(...) nms <- names2(vals) if ("default" %in% nms) { - warning( - "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", - "This warning will be removed when with_defaults() is fully deprecated." - ) - defaults <- vals$default - vals$default <- NULL - valid_idx <- nms != "default" - nms <- nms[valid_idx] + sys_calls <- sys.calls() + prev_call_id <- length(sys_calls) - 1L + # getting default= is a lot harder for modify_defaults() because defaults= is a required + # argument. so assume that this was intentional and restrict focus to linters_with_defaults calls + if ( + prev_call_id > 0L && ( + (is.name(prev_call <- sys_calls[[prev_call_id]][[1L]]) && identical(prev_call, quote(linters_with_defaults))) || + (is.call(prev_call) && identical(prev_call, quote(lintr::linters_with_defaults))) + ) + ) { + warning( + "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", + "This warning will be removed when with_defaults() is fully deprecated." + ) + defaults <- vals$default + vals$default <- NULL + valid_idx <- nms != "default" + nms <- nms[valid_idx] + } else { + valid_idx <- TRUE + } } else { valid_idx <- TRUE } diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 9dda359abf..1456e38b5b 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -63,6 +63,11 @@ test_that("modify_defaults works", { }) test_that("linters_with_defaults(default = .) is supported with a deprecation warning", { - expect_warning(linters <- linters_with_defaults(default = list(), no_tab_linter())) + expect_warning(linters <- linters_with_defaults(default = list(), no_tab_linter()), "'default'") expect_named(linters, "no_tab_linter") + + # avoid triggering the same warning in modify_defaults, even though the warning is implemented + # there, because getting default= passed takes more effort + expect_silent(linters <- modify_defaults(defaults = list(), default = list(), no_tab_linter())) + expect_named(linters, c("default", "no_tab_linter")) }) From 68a81f2f80f6a9315eb1cfed708a46940e13b4c6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 2 Jun 2022 01:14:19 +0000 Subject: [PATCH 4/4] re-implement inside linters_with_defaults only --- R/with.R | 73 +++++++++++++++++--------------------- tests/testthat/test-with.R | 8 +++-- 2 files changed, 38 insertions(+), 43 deletions(-) diff --git a/R/with.R b/R/with.R index 7072c40f22..8b2c7900ad 100644 --- a/R/with.R +++ b/R/with.R @@ -24,47 +24,9 @@ modify_defaults <- function(defaults, ...) { } vals <- list(...) nms <- names2(vals) - if ("default" %in% nms) { - sys_calls <- sys.calls() - prev_call_id <- length(sys_calls) - 1L - # getting default= is a lot harder for modify_defaults() because defaults= is a required - # argument. so assume that this was intentional and restrict focus to linters_with_defaults calls - if ( - prev_call_id > 0L && ( - (is.name(prev_call <- sys_calls[[prev_call_id]][[1L]]) && identical(prev_call, quote(linters_with_defaults))) || - (is.call(prev_call) && identical(prev_call, quote(lintr::linters_with_defaults))) - ) - ) { - warning( - "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", - "This warning will be removed when with_defaults() is fully deprecated." - ) - defaults <- vals$default - vals$default <- NULL - valid_idx <- nms != "default" - nms <- nms[valid_idx] - } else { - valid_idx <- TRUE - } - } else { - valid_idx <- TRUE - } - missing <- !nzchar(nms, keepNA = TRUE) - if (any(missing)) { - args <- as.character(eval(substitute(alist(...)[missing])))[valid_idx] - # foo_linter(x=1) => "foo" - # var[["foo"]] => "foo" - nms[missing] <- re_substitutes( - re_substitutes( - # Very long input might have newlines which are not caught - # by . in a perl regex; see #774 - re_substitutes(args, rex("(", anything), "", options = "s"), - rex(start, anything, '["' %or% "::"), - "" - ), - rex('"]', anything, end), - "" - ) + missing_index <- !nzchar(nms, keepNA = TRUE) + if (any(missing_index)) { + nms[missing_index] <- guess_names(..., missing_index = missing_index) } to_null <- vapply(vals, is.null, logical(1L)) @@ -185,6 +147,22 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep #' absolute_path_linter() #' ) linters_with_defaults <- function(..., defaults = default_linters) { + dots <- list(...) + if (missing(defaults) && "default" %in% names(dots)) { + warning( + "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", + "This warning will be removed when with_defaults() is fully deprecated." + ) + defaults <- dots$default + nms <- names2(dots) + missing_index <- !nzchar(nms, keepNA = TRUE) + if (any(missing_index)) { + names(dots)[missing_index] <- guess_names(..., missing_index = missing_index) + } + dots$default <- NULL + dots <- c(dots, list(defaults = defaults)) + return(do.call(modify_defaults, dots)) + } modify_defaults(..., defaults = defaults) } @@ -206,3 +184,16 @@ call_linter_factory <- function(linter_factory, linter_name, package) { attr(linter, "name") <- linter_name linter } + +guess_names <- function(..., missing_index) { + args <- as.character(eval(substitute(alist(...)[missing_index]))) + # foo_linter(x=1) => "foo" + # var[["foo"]] => "foo" + # strip call: foo_linter(x=1) --> foo_linter + # NB: Very long input might have newlines which are not caught + # by . in a perl regex; see #774 + args <- re_substitutes(args, rex("(", anything), "", options = "s") + # strip extractors: pkg::foo_linter, var[["foo_linter"]] --> foo_linter + args <- re_substitutes(args, rex(start, anything, '["' %or% "::"), "") + re_substitutes(args, rex('"]', anything, end), "") +} diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 1456e38b5b..f9fcc29155 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -66,8 +66,12 @@ test_that("linters_with_defaults(default = .) is supported with a deprecation wa expect_warning(linters <- linters_with_defaults(default = list(), no_tab_linter()), "'default'") expect_named(linters, "no_tab_linter") - # avoid triggering the same warning in modify_defaults, even though the warning is implemented - # there, because getting default= passed takes more effort + # the same warning is not triggered in modify_defaults expect_silent(linters <- modify_defaults(defaults = list(), default = list(), no_tab_linter())) expect_named(linters, c("default", "no_tab_linter")) + + # if default= is explicitly provided alongside defaults=, assume that was intentional + default <- Linter(function(.) list()) + expect_silent(linters <- linters_with_defaults(defaults = list(), default = default)) + expect_named(linters, "default") })