From 852e7cc51e49d26853b1ecc50ace08026785ae92 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 1 Jul 2023 03:45:19 -0700 Subject: [PATCH] fully deprecate positional arguments in lint()+friends (#1992) --- NEWS.md | 1 + R/lint.R | 52 +++++++------------------------ tests/testthat/test-dir_linters.R | 7 ++--- tests/testthat/test-lint_file.R | 7 ++--- 4 files changed, 17 insertions(+), 50 deletions(-) diff --git a/NEWS.md b/NEWS.md index f844aa081..41b653312 100644 --- a/NEWS.md +++ b/NEWS.md @@ -8,6 +8,7 @@ * `consecutive_stopifnot_linter()` is deprecated in favor of the more general (see below) `consecutive_assertion_linter()` (#1604, @MichaelChirico). * `no_tab_linter()` is deprecated in favor of `whitespace_linter()` for naming consistency and future generalization (#1954, @MichaelChirico). * `available_linters()` prioritizes `tags` over `exclude_tags` in the case of overlap, i.e., tags listed in both arguments are included, not excluded. We don't expect many people to be affected by this, and the old behavior was not made explicit in the documentation, but make note of it here since it required changing a test in lintr's own suite where `linters_with_tags()` implicitly assumed this behavior. +* `lint()`, `lint_dir()`, and `lint_package()` no longer accept certain arguments (`cache=` for `lint()`, `relative_path=` for the latter two) positionally. The `warning()` since 3.0.0 has been upgraded to an error. ## Bug fixes diff --git a/R/lint.R b/R/lint.R index 95594b64e..b453f62ab 100644 --- a/R/lint.R +++ b/R/lint.R @@ -35,15 +35,8 @@ #' #' @export lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = TRUE, text = NULL) { - # TODO(next release after 3.0.0): remove this deprecated workaround - dots <- list(...) - if (has_positional_logical(dots)) { - warning( - "'cache' is no longer available as a positional argument; please supply 'cache' as a named argument instead. ", - "This warning will be upgraded to an error in the next release." - ) - cache <- dots[[1L]] - dots <- dots[-1L] + if (has_positional_logical(list(...))) { + stop("'cache' is no longer available as a positional argument; please supply 'cache' as a named argument instead.") } needs_tempfile <- missing(filename) || rex::re_matches(filename, rex::rex(newline)) @@ -75,9 +68,7 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = lint_obj <- define_cache_key(filename, inline_data, lines) lints <- retrieve_file(lint_cache, lint_obj, linters) if (!is.null(lints)) { - # TODO: once cache= is fully deprecated as 3rd positional argument (see top of body), we can restore the cleaner: - # > exclude(lints, lines = lines, ...) - return(do.call(exclude, c(list(lints, lines = lines, linter_names = names(linters)), dots))) + return(exclude(lints, lines = lines, linter_names = names(linters), ...)) } lints <- list() @@ -102,9 +93,7 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = cache_file(lint_cache, filename, linters, lints) save_cache(lint_cache, filename, cache_path) - # TODO: once cache= is fully deprecated as 3rd positional argument (see top of body), we can restore the cleaner: - # > exclude(lints, lines = lines, ...) - res <- do.call(exclude, c(list(lints, lines = lines, linter_names = names(linters)), dots)) + res <- exclude(lints, lines = lines, linter_names = names(linters), ...) # simplify filename if inline zap_temp_filename(res, needs_tempfile) @@ -138,16 +127,11 @@ lint_dir <- function(path = ".", ..., exclusions = list("renv", "packrat"), pattern = rex::rex(".", one_of("Rr"), or("", "html", "md", "nw", "rst", "tex", "txt"), end), parse_settings = TRUE) { - # TODO(next release after 3.0.0): remove this deprecated workaround - dots <- list(...) - if (has_positional_logical(dots)) { - warning( + if (has_positional_logical(list(...))) { + stop( "'relative_path' is no longer available as a positional argument; ", - "please supply 'relative_path' as a named argument instead. ", - "This warning will be upgraded to an error in the next release." + "please supply 'relative_path' as a named argument instead. " ) - relative_path <- dots[[1L]] - dots <- dots[-1L] } if (isTRUE(parse_settings)) { @@ -179,9 +163,7 @@ lint_dir <- function(path = ".", ..., files, function(file) { maybe_report_progress() - # TODO: once relative_path= is fully deprecated as 2nd positional argument (see top of body), restore the cleaner: - # > lint(file, ..., parse_settings = FALSE, exclusions = exclusions) - do.call(lint, c(list(file, parse_settings = FALSE, exclusions = exclusions), dots)) + lint(file, ..., parse_settings = FALSE, exclusions = exclusions) } )) @@ -230,16 +212,11 @@ lint_package <- function(path = ".", ..., relative_path = TRUE, exclusions = list("R/RcppExports.R"), parse_settings = TRUE) { - # TODO(next release after 3.0.0): remove this deprecated workaround - dots <- list(...) - if (has_positional_logical(dots)) { - warning( + if (has_positional_logical(list(...))) { + stop( "'relative_path' is no longer available as a positional argument; ", - "please supply 'relative_path' as a named argument instead. ", - "This warning will be upgraded to an error in the next release." + "please supply 'relative_path' as a named argument instead. " ) - relative_path <- dots[[1L]] - dots <- dots[-1L] } if (length(path) > 1L) { @@ -263,12 +240,7 @@ lint_package <- function(path = ".", ..., ) r_directories <- file.path(pkg_path, c("R", "tests", "inst", "vignettes", "data-raw", "demo", "exec")) - # TODO: once relative_path= is fully deprecated as 2nd positional argument (see top of body), restore the cleaner: - # > lints <- lint_dir(r_directories, relative_path = FALSE, exclusions = exclusions, parse_settings = FALSE, ...) - lints <- do.call( - lint_dir, - c(list(r_directories, relative_path = FALSE, exclusions = exclusions, parse_settings = FALSE), dots) - ) + lints <- lint_dir(r_directories, relative_path = FALSE, exclusions = exclusions, parse_settings = FALSE, ...) if (isTRUE(relative_path)) { path <- normalizePath(pkg_path, mustWork = FALSE) diff --git a/tests/testthat/test-dir_linters.R b/tests/testthat/test-dir_linters.R index 9bb3b1d85..2080ab304 100644 --- a/tests/testthat/test-dir_linters.R +++ b/tests/testthat/test-dir_linters.R @@ -96,12 +96,9 @@ test_that("lint_dir works with specific linters without specifying other argumen test_that("lint_dir continues to accept relative_path= in 2nd positional argument, with a warning", { the_dir <- test_path("dummy_packages", "package", "vignettes") - expect_warning( - { - positional_lints <- lint_dir(the_dir, FALSE) - }, + expect_error( + lint_dir(the_dir, FALSE), "'relative_path' is no longer available as a positional argument", fixed = TRUE ) - expect_identical(positional_lints, lint_dir(the_dir, relative_path = FALSE)) }) diff --git a/tests/testthat/test-lint_file.R b/tests/testthat/test-lint_file.R index 53a344140..8b70661bc 100644 --- a/tests/testthat/test-lint_file.R +++ b/tests/testthat/test-lint_file.R @@ -220,14 +220,11 @@ test_that("compatibility warnings work", { }) test_that("Deprecated positional usage of cache= works, with warning", { - expect_warning( - { - l <- lint("a = 2\n", FALSE, linters = assignment_linter()) - }, + expect_error( + lint("a = 2\n", FALSE, linters = assignment_linter()), "'cache' is no longer available as a positional argument", fixed = TRUE ) - expect_identical(l, lint("a = 2\n", assignment_linter(), cache = FALSE)) }) test_that("Linters throwing an error give a helpful error", {