more considered handling of conditions in config parsing

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
 

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
 }
 

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")
+})