Change ... precedence in lint(), lint_dir(), lint_package() (#937)

* reorder ... args in lint/lint_dir/lint_package

* add a test

* debug vestige

* soften deprecation for lint()

* unparseable comment

* fixes for lint_dir and lint_pckage

* test of positional usage of relative_path in test_dir

* catch specific warning

* add a test for lint()

Co-authored-by: AshesITR <alexander.rosenstock@web.de>

NEWS.md

@@ -1,5 +1,19 @@
 # lintr (development version)
 
+## Potentially breaking changes and pending deprecations
+
+* Lints are now marked with the name of the `linter` that caused them instead of the name of their implementation
+  function.
+  Deprecated the obsolete `linter` argument of `Lint()`. (#664, #673, #746, @AshesITR)
+  Downstream custom linters should follow suit.
+* Consistent access to linters through a function call, even for linters without parameters (#245, @fangly, @AshesITR, and @MichaelChirico)
+* Removed deprecated functions `absolute_paths_linter`, `camel_case_linter`, `multiple_dots_linter`, `snake_case_linter`, and `trailing_semicolons_linter`. They have been marked as deprecated since v1.0.1, which was released in 2017.
+* The `...` arguments for `lint()`, `lint_dir()`, and `lint_package()` have promoted to an earlier position to better match the [Tidyverse design principal](https://design.tidyverse.org/args-data-details.html) of data->descriptor->details. This change enables passing objects to `...` without needing to specify non-required arguments, e.g. `lint_dir("/path/to/dir", linter())` now works without the need to specify `relative_path`. This affects some code that uses positional arguments. (#935, @michaelchirico)
+  + For `lint()`, `...` is now the 3rd argument, where earlier this was `cache=`
+  + For `lint_dir()` and `lint_package()`, `...` is now the 2nd argument, where earlier this was `relative_path=`
+
+## New features, bug fixes, improvements
+
 * Updated R CMD GitHub Actions workflow to check for R 3.6 on Ubuntu, instead of R 3.3, and for R 4.0 on Windows, instead of R 3.6 (#803, @ dragosmg)
 * Added a secondary, more restrictive lint workflow - `lint-changed-files` - for newly written / modified code (#641, @dragosmg) 
 * Switched CI from Travis to GitHub Actions, using the full tidyverse recommended R CMD check. Code coverage and linting 
@@ -36,9 +50,6 @@
   excluding matches found in comments; #441 and #545, @russHyde)
 * `paren_brace_linter` now marks lints at the opening brace instead of the closing parenthesis, making fixing the lints
   by jumping to source markers easier (#583, @AshesITR)
-* Lints are now marked with the name of the `linter` that caused them instead of the name of their implementation
-  function.    
-  Deprecated the obsolete `linter` argument of `Lint()`. (#664, #673, #746, @AshesITR)
 * New syntax to exclude only selected linters from linting lines or passages. Use `# nolint: linter_name, linter2_name.`
   or `# nolint start: linter_name, linter2_name.` in source files or named lists of line numbers in `.lintr`.
   (#660, @AshesITR)
@@ -54,7 +65,6 @@
 * `commented_code_linter()` uses the parse tree to find comments, eliminating some false positives (#451, @AshesITR)
 * `trailing_blank_lines_linter()` now also lints files without a terminal newline (#675, @AshesITR)
 * `object_name_linter()` now correctly detects imported functions when linting packages (#642, @AshesITR)
-* Consistent access to linters through a function call, even for linters without parameters (#245, @fangly, @AshesITR, and @MichaelChirico)
 * `object_usage_linter()` now correctly reports usage warnings spanning multiple lines (#507, @AshesITR)
 * `T_and_F_symbol_linter()` no longer lints occurrences of `T` and `F` when used for subsetting and gives a better 
   message when used as variable names (#657, @AshesITR)
@@ -72,7 +82,6 @@
 * `undesirable_function_linter` no longer lints `library` and `require` calls attaching a package with an undesired name, e.g. `library(foo)` (#814, @kpagacz and @michaelchirico)
 * New linter `duplicate_argument_linter()` checks that there are no duplicate arguments supplied to
 function calls. (#850, #851, @renkun-ken)
-* Removed deprecated functions `absolute_paths_linter`, `camel_case_linter`, `multiple_dots_linter`, `snake_case_linter`, and `trailing_semicolons_linter`. They have been marked as deprecated since v1.0.1, which was released in 2017.
 * Several optional `Imported` packages have become `Suggested` dependencies: `httr`, `testthat`, and `rstudioapi`. This should allow snappier CI builds for usages not relying on some more "peripheral" features of the package.
 * Error message for mismatched starts and ends of exclusion ranges is now more helpful. (#571, #860, @AshesITR and @danielinteractive)
 * Debugging functions (`browser()`, `debug()`, `debugcall()`, `debugonce()`, `trace()`, `undebug()`, `untrace()`) are now part of the default set of undesirable functions to help prevent them from being committed by mistake. (#876, @michaelchirico)

R/lint.R

@@ -18,9 +18,9 @@ NULL
 #' The latter (inline data) applies whenever `filename` has a newline character (\\n).
 #' @param linters a named list of linter functions to apply see [linters] for a full list of default and available
 #' linters.
+#' @param ... additional arguments passed to [exclude()].
 #' @param cache given a logical, toggle caching of lint results. If passed a character string, store the cache in this
 #' directory.
-#' @param ... additional arguments passed to [exclude()].
 #' @param parse_settings whether to try and parse the settings.
 #' @param text Optional argument for supplying a string or lines directly, e.g. if the file is already in memory or
 #' linting is being done ad hoc.
@@ -35,8 +35,17 @@ NULL
 #' }
 #'
 #' @export
-lint <- function(filename, linters = NULL, cache = FALSE, ..., parse_settings = TRUE, text = NULL) {
-
+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 (length(dots) > 0L && is.logical(dots[[1L]])) {
+    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 (is.null(text)) {
     inline_data <- rex::re_matches(filename, rex::rex(newline))
     if (inline_data) {
@@ -93,7 +102,9 @@ lint <- function(filename, linters = NULL, cache = FALSE, ..., parse_settings =
     lint_obj <- if (is.null(text)) filename else list(content = get_content(lines), TRUE)
     lints <- retrieve_file(lint_cache, lint_obj, linters)
     if (!is.null(lints)) {
-      return(exclude(lints, lines = lines, ...))
+      # 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), dots)))
     }
     cache <- TRUE
   } else {
@@ -139,7 +150,9 @@ lint <- function(filename, linters = NULL, cache = FALSE, ..., parse_settings =
     save_cache(lint_cache, filename, cache_path)
   }
 
-  res <- exclude(lints, lines = lines, ...)
+  # 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), dots))
 
   # simplify filename if inline
   if (no_filename) {
@@ -156,9 +169,9 @@ lint <- function(filename, linters = NULL, cache = FALSE, ..., parse_settings =
 #'
 #' @param path the path to the base directory, by default, it will be searched in the parent directories of the current
 #' directory.
+#' @param ... additional arguments passed to [lint()], e.g. `linters` or `cache`.
 #' @param relative_path if `TRUE`, file paths are printed using their path relative to the base directory.
 #'   If `FALSE`, use the full absolute path.
-#' @param ... additional arguments passed to [lint()], e.g. `cache` or `linters`.
 #' @param exclusions exclusions for [exclude()], relative to the package path.
 #' @param pattern pattern for files, by default it will take files with any of the extensions .R, .Rmd, .Rnw, .Rhtml,
 #' .Rrst, .Rtex, .Rtxt allowing for lowercase r (.r, ...)
@@ -174,13 +187,22 @@ lint <- function(filename, linters = NULL, cache = FALSE, ..., parse_settings =
 #'   )
 #' }
 #' @export
-lint_dir <- function(path = ".", relative_path = TRUE, ..., exclusions = list("renv", "packrat"),
-                     pattern = rex::rex(
-                       ".", one_of("Rr"),
-                       or("", "html", "md", "nw", "rst", "tex", "txt"),
-                       end
-                     ),
+lint_dir <- function(path = ".", ...,
+                     relative_path = TRUE,
+                     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 (length(dots) > 0L && is.logical(dots[[1L]])) {
+    warning(
+      "'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."
+    )
+    relative_path <- dots[[1L]]
+    dots <- dots[-1L]
+  }
 
   if (isTRUE(parse_settings)) {
     read_settings(path)
@@ -221,7 +243,9 @@ lint_dir <- function(path = ".", relative_path = TRUE, ..., exclusions = list("r
       if (interactive() && !identical(Sys.getenv("TESTTHAT"), "true")) {
         message(".", appendLF = FALSE) # nocov
       }
-      lint(file, ..., parse_settings = FALSE, exclusions = exclusions)
+      # 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))
     }
   ))
 
@@ -268,8 +292,22 @@ lint_dir <- function(path = ".", relative_path = TRUE, ..., exclusions = list("r
 #'   )
 #' }
 #' @export
-lint_package <- function(path = ".", relative_path = TRUE, ...,
-                         exclusions = list("R/RcppExports.R"), parse_settings = TRUE) {
+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 (length(dots) > 0L && is.logical(dots[[1L]])) {
+    warning(
+      "'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."
+    )
+    relative_path <- dots[[1L]]
+    dots <- dots[-1L]
+  }
+
   pkg_path <- find_package(path)
 
   if (is.null(pkg_path)) {
@@ -287,8 +325,13 @@ lint_package <- function(path = ".", relative_path = TRUE, ...,
     root = pkg_path
   )
 
-  lints <- lint_dir(file.path(pkg_path, c("R", "tests", "inst", "vignettes", "data-raw", "demo")),
-                    relative_path = FALSE, exclusions = exclusions, parse_settings = FALSE, ...)
+  r_directories <- file.path(pkg_path, c("R", "tests", "inst", "vignettes", "data-raw", "demo"))
+  # 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)
+  )
 
   if (isTRUE(relative_path)) {
     path <- normalizePath(pkg_path, mustWork = FALSE)

tests/testthat/test-dir_linters.R

@@ -82,3 +82,19 @@ test_that("respect directory exclusions from settings", {
   linted_files <- unique(names(lints))
   expect_length(linted_files, 1L)
 })
+
+test_that("lint_dir works with specific linters without specifying other arguments", {
+  the_dir <- file.path("dummy_packages", "package", "vignettes")
+  expect_length(lint_dir(the_dir, assignment_linter(), parse_settings = FALSE), 12L)
+  expect_length(lint_dir(the_dir, commented_code_linter(), parse_settings = FALSE), 0L)
+})
+
+test_that("lint_dir continues to accept relative_path= in 2nd positional argument, with a warning", {
+  the_dir <- file.path("dummy_packages", "package", "vignettes")
+  expect_warning(
+    positional_lints <- 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))
+})

tests/testthat/test-lint_file.R

@@ -205,3 +205,12 @@ test_that("compatibility warnings work", {
     regexp = rex("Expected '", anything, "' to be of class 'linter'")
   )
 })
+
+test_that("Deprecated positional usage of cache= works, with warning", {
+  expect_warning(
+    l <- 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))
+})