From 18ab466e0a38a74e5743e22830a8058d32764541 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 3 Nov 2023 23:14:18 -0700 Subject: [PATCH] more considered handling of conditions in config parsing --- NEWS.md | 2 +- R/settings.R | 20 ++++++++++++---- tests/testthat/test-settings.R | 42 ++++++++++++++++++++++------------ 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/NEWS.md b/NEWS.md index a4d533834..7b10cb656 100644 --- a/NEWS.md +++ b/NEWS.md @@ -5,7 +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. + + 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. Thanks also to @Bisaloo for early testing to improve the failure interface for invalid configs (#2253). ## Bug fixes diff --git a/R/settings.R b/R/settings.R index 3aed60713..1352f6e83 100644 --- a/R/settings.R +++ b/R/settings.R @@ -64,17 +64,27 @@ read_config_file <- function(config_file) { load_config <- function(file) { dcf_values <- read.dcf(file, all = TRUE) for (setting in names(dcf_values)) { - tryCatch( - assign(setting, eval(str2lang(dcf_values[[setting]])), envir = config), - error = function(e) stop("Malformed config setting '", setting, "'\n ", conditionMessage(e), call. = FALSE) + parsed_setting <- tryCatch( + str2lang(dcf_values[[setting]]), + error = function(e) stop("Malformed config setting '", setting, "':\n ", conditionMessage(e), call. = FALSE) ) + setting_value <- tryCatch( + eval(parsed_setting), + warning = function(w) warning("Warning from config setting '", setting, "':\n ", conditionMessage(w), call. = FALSE), + error = function(e) stop("Error from config setting '", setting, "':\n ", conditionMessage(e), call. = FALSE) + ) + assign(setting, eval(parsed_setting), envir = config) } } malformed <- function(e) { - stop("Malformed config file, ensure it ends in a newline\n ", conditionMessage(e), call. = FALSE) + stop("Malformed config file:\n ", conditionMessage(e), call. = FALSE) } } - tryCatch(load_config(config_file), warning = malformed, error = malformed) + tryCatch( + load_config(config_file), + warning = function(w) warning("Warning encountered while loading config:\n ", conditionMessage(w), call. = FALSE), + error = malformed + ) config } diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index cd9cdbabd..cee85c505 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -60,23 +60,28 @@ test_that("it uses system config directory settings if provided", { expect_identical(settings$exclude, "test") }) -test_that("it errors if the config file does not end in a newline", { - f <- withr::local_tempfile() - cat("linters: linters_with_defaults(closed_curly_linter = NULL)", file = f) - withr::local_options(list(lintr.linter_file = f)) - expect_error(lintr:::read_settings("foo"), "Malformed config file") +test_that("read_config_file() warns if the config file does not end in a newline", { + .lintr <- withr::local_tempfile() + withr::local_options(lintr.linter_file = .lintr) + withr::local_dir(withr::local_tempdir()) + + # cat() not writeLines() to ensure no trailing \n + cat("linters: modify_defaults(braces_linter = NULL)", file = .lintr) + writeLines("a <- 1", "aaa.R") + expect_warning(lint_dir(), "Warning encountered while loading config", fixed = TRUE) }) test_that("it gives informative errors if the config file contains errors", { - f <- withr::local_tempfile( - lines = c( - "linters: linters_with_defaults(", - " closed_curly_linter = NULL,", - " )" - ) - ) - withr::local_options(list(lintr.linter_file = f)) - expect_error(lintr:::read_settings("foo"), "Malformed config setting 'linters'") + .lintr <- withr::local_tempfile(lines = c( + "linters: modify_defaults(", + " braces_linter = NULL,", + " )" + )) + withr::local_options(lintr.linter_file = .lintr) + withr::local_dir(withr::local_tempdir()) + + writeLines("a <- 1", "aaa.R") + expect_error(lint_dir(), "Error from config setting 'linters'", fixed = TRUE) }) test_that("rot utility works as intended", { @@ -259,3 +264,12 @@ test_that("lines Inf means 'all lines'", { writeLines("a=1", "aaa.R") expect_length(lint_dir(linters = list(assignment_linter(), infix_spaces_linter())), 1L) }) + +test_that("read_config_file() bubbles up warnings helpfully, without erroring (#2253)", { + .lintr <- withr::local_tempfile(lines = 'linters: list(backport_linter("2.0.0"))') + withr::local_options(lintr.linter_file = .lintr) + withr::local_dir(withr::local_tempdir()) + + writeLines("a <- 1", "aaa.R") + expect_warning(lint_dir(), "Warning from config setting 'linters'.*Resetting 'r_version' to 3.0.0") +})