diff --git a/NEWS.md b/NEWS.md index e6e5d30469..07073e65b6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,6 +5,7 @@ * `infix_spaces_linter()` distinguishes `<-`, `:=`, `<<-` and `->`, `->>`, i.e. `infix_spaces_linter(exclude_operators = "->")` will no longer exclude `->>` (#2115, @MichaelChirico). This change is breaking for users relying on manually-supplied `exclude_operators` containing `"<-"` to also exclude `:=` and `<<-`. The fix is to manually supply `":="` and `"<<-"` as well. We don't expect this change to affect many users, the fix is simple, and the new behavior is much more transparent, so we are including this breakage in a minor release. * Removed `find_line()` and `find_column()` entries from `get_source_expressions()` expression-level objects. These have been marked deprecated since version 3.0.0. No users were found on GitHub. * There is experimental support for writing config in plain R scripts (as opposed to DCF files; #1210, @MichaelChirico). The script is run in a new environment and variables matching settings (`?default_settings`) are copied over. In particular, this removes the need to write R code in a DCF-friendly way, and allows normal R syntax highlighting in the saved file. We may eventually deprecate the DCF approach in favor of this one; user feedback is welcome on strong preferences for either approach, or for a different approach like YAML. Generally you should be able to convert your existing `.lintr` file to an equivalent R config by replacing the `:` key-value separators with assignments (`<-`). By default, such a config is searched for in a file named '.lintr.R'. This is a mildly breaking change if you happened to be keeping a file '.lintr.R' around since that file is given precedence over '.lintr'. + + We also validate config files up-front make it clearer when invalid configs are present (#2195, @MichaelChirico). There is a warning for "invalid" settings, i.e., settings not part of `?default_settings`. We think this is more likely to affect users declaring settings in R, since any variable defined in the config that's not a setting must be removed to make it clearer which variables are settings vs. ancillary. ## Bug fixes diff --git a/R/exclude.R b/R/exclude.R index ae5901f1d1..d43348a403 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -10,18 +10,25 @@ #' "@details", #' "Exclusions can be specified in three different ways.", #' "", -#' "1. single line in the source file. default: `# nolint`, possibly followed by a listing of linters to exclude.", +#' "1. Single line in the source file. default: `# nolint`, possibly followed by a listing of linters to exclude.", #' " If the listing is missing, all linters are excluded on that line. The default listing format is", #' paste( #' " `#", #' "nolint: linter_name, linter2_name.`. There may not be anything between the colon and the line exclusion tag" #' ), #' " and the listing must be terminated with a full stop (`.`) for the linter list to be respected.", -#' "2. line range in the source file. default: `# nolint start`, `# nolint end`. `# nolint start` accepts linter", +#' "2. Line range in the source file. default: `# nolint start`, `# nolint end`. `# nolint start` accepts linter", #' " lists in the same form as `# nolint`.", -#' "3. exclusions parameter, a named list of files with named lists of linters and lines to exclude them on, a named", -#' " list of the files and lines to exclude, or just the filenames if you want to exclude the entire file, or the", -#' " directory names if you want to exclude all files in a directory." +#' "3. Exclusions parameter, a list with named and/or unnamed entries. ", +#' " Outer elements have the following characteristics:", +#' " 1. Unnamed elements specify filenames or directories.", +#' " 2. Named elements are a vector or list of line numbers, with `Inf` indicating 'all lines'.", +#' " The name gives a path relative to the config.", +#' " 1. Unnamed elements denote exclusion of all linters in the given path or directory.", +#' " 2. Named elements, where the name specifies a linter, denote exclusion for that linter.", +#' " For convenience, a vector can be used in place of a list whenever it would not introduce ambiguity, e.g.", +#' " a character vector of files to exclude or a vector of lines to exclude.", +#' NULL #' ) exclude <- function(lints, exclusions = settings$exclusions, linter_names = NULL, ...) { if (length(lints) <= 0L) { diff --git a/R/settings.R b/R/settings.R index 8addc45e7e..3aed607135 100644 --- a/R/settings.R +++ b/R/settings.R @@ -32,6 +32,7 @@ read_settings <- function(filename) { } config <- read_config_file(config_file) + validate_config_file(config, config_file, default_settings) for (setting in names(default_settings)) { value <- get_setting(setting, config, default_settings) @@ -77,6 +78,97 @@ read_config_file <- function(config_file) { config } +validate_config_file <- function(config, config_file, defaults) { + matched <- names(config) %in% names(defaults) + if (!all(matched)) { + warning("Found unused settings in config '", config_file, "': ", toString(names(config)[!matched])) + } + + validate_regex(config, + c("exclude", "exclude_next", "exclude_start", "exclude_end", "exclude_linter", "exclude_linter_sep") + ) + validate_character_string(config, c("encoding", "cache_directory", "comment_token")) + validate_true_false(config, c("comment_bot", "error_on_lint")) + validate_linters(config$linters) + validate_exclusions(config$exclusions) +} + +is_character_string <- function(x) is.character(x) && length(x) == 1L && !is.na(x) +is_valid_regex <- function(str) !inherits(tryCatch(grepl(str, ""), condition = identity), "condition") +is_single_regex <- function(x) is_character_string(x) && is_valid_regex(x) +is_true_false <- function(x) is.logical(x) && length(x) == 1L && !is.na(x) + +validate_keys <- function(config, keys, test, what) { + for (key in keys) { + val <- config[[key]] + if (is.null(val)) { + next + } + if (!test(val)) { + stop("Setting '", key, "' should be ", what, ", not '", toString(val), "'.") + } + } +} + +validate_regex <- function(config, keys) { + validate_keys(config, keys, is_single_regex, "a single regular expression") +} + +validate_character_string <- function(config, keys) { + validate_keys(config, keys, is_character_string, "a character string") +} + +validate_true_false <- function(config, keys) { + validate_keys(config, keys, is_true_false, "TRUE or FALSE") +} + +validate_linters <- function(linters) { + if (is.null(linters)) { + return(invisible()) + } + + is_linters <- vapply(linters, is_linter, logical(1L)) + if (!all(is_linters)) { + stop( + "Setting 'linters' should be a list of linters, but found non-linters at elements ", + toString(which(!is_linters)), "." + ) + } +} + +validate_exclusions <- function(exclusions) { + if (is.null(exclusions)) { + return(invisible()) + } + + exclusion_names <- names2(exclusions) + has_names <- exclusion_names != "" + unnamed_is_string <- + vapply(exclusions[!has_names], function(x) is.character(x) && length(x) == 1L && !is.na(x), logical(1L)) + if (!all(unnamed_is_string)) { + stop( + "Unnamed entries of setting 'exclusions' should be strings naming files or directories, check entries: ", + toString(which(!has_names)[!unnamed_is_string]), "." + ) + } + for (ii in which(has_names)) validate_named_exclusion(exclusions, ii) +} + +validate_named_exclusion <- function(exclusions, idx) { + entry <- exclusions[[idx]] + if (is.list(entry)) { + valid_entry <- vapply(entry, function(x) is.numeric(x) && !anyNA(x), logical(1L)) + } else { + valid_entry <- is.numeric(entry) && !anyNA(entry) + } + if (!all(valid_entry)) { + stop( + "Named entries of setting 'exclusions' should designate line numbers for exclusion, ", + "check exclusion: ", idx, "." + ) + } +} + lintr_option <- function(setting, default = NULL) getOption(paste0("lintr.", setting), default) get_setting <- function(setting, config, defaults) { diff --git a/R/zzz.R b/R/zzz.R index b9d6f4b7c2..226063c9b0 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -250,7 +250,7 @@ default_undesirable_operators <- all_undesirable_operators[names(all_undesirable #' - `exclude`: pattern used to exclude a line of code #' - `exclude_start`, `exclude_end`: patterns used to mark start and end of the code block to exclude #' - `exclude_linter`, `exclude_linter_sep`: patterns used to exclude linters -#' - `exclusions`:a list of files to exclude +#' - `exclusions`: a list of exclusions, see [exclude()] for a complete description of valid values. #' - `cache_directory`: location of cache directory #' - `comment_token`: a GitHub token character #' - `comment_bot`: decides if lintr comment bot on GitHub can comment on commits diff --git a/man/default_settings.Rd b/man/default_settings.Rd index 11c92cb35d..d002398168 100644 --- a/man/default_settings.Rd +++ b/man/default_settings.Rd @@ -23,7 +23,7 @@ The default settings consist of \item \code{exclude}: pattern used to exclude a line of code \item \code{exclude_start}, \code{exclude_end}: patterns used to mark start and end of the code block to exclude \item \code{exclude_linter}, \code{exclude_linter_sep}: patterns used to exclude linters -\item \code{exclusions}:a list of files to exclude +\item \code{exclusions}: a list of exclusions, see \code{\link[=exclude]{exclude()}} for a complete description of valid values. \item \code{cache_directory}: location of cache directory \item \code{comment_token}: a GitHub token character \item \code{comment_bot}: decides if lintr comment bot on GitHub can comment on commits diff --git a/man/exclude.Rd b/man/exclude.Rd index 17ccdaba2d..3a97087e19 100644 --- a/man/exclude.Rd +++ b/man/exclude.Rd @@ -21,14 +21,24 @@ Exclude lines or files from linting \details{ Exclusions can be specified in three different ways. \enumerate{ -\item single line in the source file. default: \verb{# nolint}, possibly followed by a listing of linters to exclude. +\item Single line in the source file. default: \verb{# nolint}, possibly followed by a listing of linters to exclude. If the listing is missing, all linters are excluded on that line. The default listing format is \verb{# nolint: linter_name, linter2_name.}. There may not be anything between the colon and the line exclusion tag and the listing must be terminated with a full stop (\code{.}) for the linter list to be respected. -\item line range in the source file. default: \verb{# nolint start}, \verb{# nolint end}. \verb{# nolint start} accepts linter +\item Line range in the source file. default: \verb{# nolint start}, \verb{# nolint end}. \verb{# nolint start} accepts linter lists in the same form as \verb{# nolint}. -\item exclusions parameter, a named list of files with named lists of linters and lines to exclude them on, a named -list of the files and lines to exclude, or just the filenames if you want to exclude the entire file, or the -directory names if you want to exclude all files in a directory. +\item Exclusions parameter, a list with named and/or unnamed entries. +Outer elements have the following characteristics: +\enumerate{ +\item Unnamed elements specify filenames or directories. +\item Named elements are a vector or list of line numbers, with \code{Inf} indicating 'all lines'. +The name gives a path relative to the config. +\enumerate{ +\item Unnamed elements denote exclusion of all linters in the given path or directory. +\item Named elements, where the name specifies a linter, denote exclusion for that linter. +For convenience, a vector can be used in place of a list whenever it would not introduce ambiguity, e.g. +a character vector of files to exclude or a vector of lines to exclude. +} +} } } diff --git a/tests/testthat/test-lint_package.R b/tests/testthat/test-lint_package.R index 598f2a9ffd..ae95bdde67 100644 --- a/tests/testthat/test-lint_package.R +++ b/tests/testthat/test-lint_package.R @@ -217,9 +217,13 @@ test_that("package using .lintr.R config lints correctly", { # config produces unused variables withr::local_options(lintr.linter_file = "lintr_test_config_extraneous") - expect_length(lint_package(r_config_pkg), 2L) + expect_warning( + expect_length(lint_package(r_config_pkg), 2L), + "Found unused settings in config", + fixed = TRUE + ) - # DCF is preferred if multiple matched configs + # R is preferred if multiple matched configs withr::local_options(lintr.linter_file = "lintr_test_config_conflict") lints <- as.data.frame(lint_package(r_config_pkg)) expect_identical(unique(basename(lints$filename)), "testthat.R") diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 7631be1dbf..cd9cdbabdc 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -152,3 +152,110 @@ test_that("it has a smart default for encodings", { lintr:::read_settings(pkg_file) expect_identical(settings$encoding, "ISO8859-1") }) + +test_that("validate_config_file() detects improperly-formed settings", { + .lintr <- withr::local_tempfile() + withr::local_options(lintr.linter_file = .lintr) + withr::local_dir(withr::local_tempdir()) + + writeLines("asdf: 1", .lintr) + expect_warning(lint_dir(), "Found unused settings in config", fixed = TRUE) + + writeLines("a=1", "aaa.R") + writeLines(c('exclusions: list("aaa.R")', "asdf: 1"), .lintr) + expect_warning(lint_dir(), "Found unused settings in config", fixed = TRUE) + + writeLines("encoding: FALSE", .lintr) + expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'FALSE'", fixed = TRUE) + + writeLines("encoding: NA_character_", .lintr) + expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'NA'", fixed = TRUE) + + writeLines('encoding: c("a", "b")', .lintr) + expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'a, b'") + + writeLines("exclude: FALSE", .lintr) + expect_error(lint_dir(), "Setting 'exclude' should be a single regular expression, not 'FALSE'", fixed = TRUE) + + writeLines(c('exclusions: list("aaa.R")', "exclude: FALSE"), .lintr) + expect_error(lint_dir(), "Setting 'exclude' should be a single regular expression, not 'FALSE'", fixed = TRUE) + + writeLines('exclude: "("', .lintr) + expect_error(lint_dir(), "Setting 'exclude' should be a single regular expression, not '('", fixed = TRUE) + + writeLines('comment_bot: "a"', .lintr) + expect_error(lint_dir(), "Setting 'comment_bot' should be TRUE or FALSE, not 'a'", fixed = TRUE) + + writeLines("comment_bot: NA", .lintr) + expect_error(lint_dir(), "Setting 'comment_bot' should be TRUE or FALSE, not 'NA'", fixed = TRUE) + + writeLines("comment_bot: c(TRUE, FALSE)", .lintr) + expect_error(lint_dir(), "Setting 'comment_bot' should be TRUE or FALSE, not 'TRUE, FALSE'", fixed = TRUE) + + writeLines("linters: list(1)", .lintr) + expect_error(lint_dir(), "Setting 'linters' should be a list of linters", fixed = TRUE) + + writeLines("linters: list(assignment_linter(), 1)", .lintr) + expect_error(lint_dir(), "Setting 'linters' should be a list of linters", fixed = TRUE) + + writeLines("exclusions: list(1L)", .lintr) + expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + + writeLines('exclusions: list("aaa.R", 1L)', .lintr) + expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + + writeLines("exclusions: list(letters)", .lintr) + expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + + writeLines("exclusions: list(NA_character_)", .lintr) + expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + + writeLines('exclusions: list(aaa.R = "abc")', .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + + writeLines("exclusions: list(aaa.R = NA_integer_)", .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + + writeLines('exclusions: list(aaa.R = list("abc"))', .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + + writeLines("exclusions: list(aaa.R = list(NA_integer_))", .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + + writeLines('exclusions: list(aaa.R = list(assignment_linter = "abc"))', .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + + writeLines("exclusions: list(aaa.R = list(assignment_linter = NA_integer_))", .lintr) + expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) +}) + +test_that("exclusions can be a character vector", { + withr::local_dir(withr::local_tempdir()) + # exclusions are relative to dirname(.lintr), so must create it here + .lintr <- withr::local_tempfile(tmpdir = getwd()) + withr::local_options(lintr.linter_file = .lintr) + + writeLines('exclusions: "aaa.R"', .lintr) + writeLines("a<-1", "aaa.R") + writeLines("b<-1", "bbb.R") + expect_length(lint_dir(linters = infix_spaces_linter()), 1L) + + writeLines('exclusions: c("aaa.R", "bbb.R")', .lintr) + expect_length(lint_dir(linters = infix_spaces_linter()), 0L) +}) + +test_that("lines Inf means 'all lines'", { + withr::local_dir(withr::local_tempdir()) + # exclusions are relative to dirname(.lintr), so must create it here + .lintr <- withr::local_tempfile(tmpdir = getwd()) + withr::local_options(lintr.linter_file = .lintr) + + writeLines("exclusions: list(aaa.R = Inf)", .lintr) + writeLines("a<-1", "aaa.R") + expect_length(lint_dir(linters = infix_spaces_linter()), 0L) + + writeLines("exclusions: list(aaa.R = list(infix_spaces_linter = Inf))", .lintr) + # exclude infix_spaces_linter, include assignment_linter() + writeLines("a=1", "aaa.R") + expect_length(lint_dir(linters = list(assignment_linter(), infix_spaces_linter())), 1L) +})