From d31e0e457cd0d8f3dd866e1f2a97ddf857092a7e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 12 Jun 2024 14:19:39 -0700 Subject: [PATCH 1/5] Bump required R version (#2603) * Bump required R version * typo Co-authored-by: Indrajeet Patil --------- Co-authored-by: Indrajeet Patil --- DESCRIPTION | 2 +- NEWS.md | 1 + tests/testthat/test-lint_package.R | 3 --- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index b22c1dd62..4831fe875 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -21,7 +21,7 @@ License: MIT + file LICENSE URL: https://lintr.r-lib.org, https://github.com/r-lib/lintr BugReports: https://github.com/r-lib/lintr/issues Depends: - R (>= 3.6) + R (>= 4.0) Imports: backports (>= 1.1.7), cli (>= 3.4.0), diff --git a/NEWS.md b/NEWS.md index 39cc847a5..f09fbe60e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -86,6 +86,7 @@ ## Notes * All user-facing messages are now prepared using the `{cli}` package (#2418, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent. +* {lintr} now depends on R version 4.0.0. It already does so implicitly due to recursive upstream dependencies requiring this version; we've simply made that dependency explicit and up-front (#2569, @MichaelChirico). # lintr 3.1.2 diff --git a/tests/testthat/test-lint_package.R b/tests/testthat/test-lint_package.R index f0d2c0c48..957b8af9c 100644 --- a/tests/testthat/test-lint_package.R +++ b/tests/testthat/test-lint_package.R @@ -231,9 +231,6 @@ test_that("package using .lintr.R config lints correctly", { }) test_that("lintr need not be attached for .lintr.R configs to use lintr functions", { - # For some obscure reason, running in the subprocess on this specific version of R - # on Windows stopped working after PR #2446 with 'Package lintr not found'. - if (getRversion() == "3.6.3") skip_on_os("windows") exprs <- paste( 'options(lintr.linter_file = "lintr_test_config")', sprintf('lints <- lintr::lint_package("%s")', test_path("dummy_packages", "RConfig")), From 1f5e46992c7f5590f6d28e17e212a861f12312f0 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 12 Jun 2024 23:04:33 -0700 Subject: [PATCH 2/5] More adaptation to 4.0+ features (#2606) * more adaptation to 4.0+ features * better future-proof docs * one more var rename --- NAMESPACE | 7 +------ R/absolute_path_linter.R | 4 +--- R/lintr-package.R | 7 +------ R/utils.R | 24 +++++++++++------------- R/zzz.R | 3 +-- man/absolute_path_linter.Rd | 5 +---- man/get_r_string.Rd | 26 ++++++++++++-------------- 7 files changed, 28 insertions(+), 48 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 767662c58..6b5c46937 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,11 +1,5 @@ # Generated by roxygen2: do not edit by hand - -if (getRversion() >= "4.0.0") { - importFrom(tools, R_user_dir) -} else { - importFrom(backports, R_user_dir) -} S3method("[",lints) S3method(as.data.frame,lints) S3method(format,lint) @@ -183,6 +177,7 @@ importFrom(rex,re_substitutes) importFrom(rex,regex) importFrom(rex,rex) importFrom(stats,na.omit) +importFrom(tools,R_user_dir) importFrom(utils,capture.output) importFrom(utils,getParseData) importFrom(utils,getTxtProgressBar) diff --git a/R/absolute_path_linter.R b/R/absolute_path_linter.R index a27ebc1ea..38fffd40b 100644 --- a/R/absolute_path_linter.R +++ b/R/absolute_path_linter.R @@ -8,9 +8,7 @@ #' * contain at least two path elements, with one having at least two characters and #' * contain only alphanumeric chars (including UTF-8), spaces, and win32-allowed punctuation #' -#' @examplesIf getRversion() >= "4.0" -#' # Following examples use raw character constant syntax introduced in R 4.0. -#' +#' @examples #' # will produce lints #' lint( #' text = 'R"--[/blah/file.txt]--"', diff --git a/R/lintr-package.R b/R/lintr-package.R index 27f55bee3..70f8d0d11 100644 --- a/R/lintr-package.R +++ b/R/lintr-package.R @@ -12,16 +12,11 @@ #' @importFrom glue glue glue_collapse #' @importFrom rex rex regex re_matches re_substitutes character_class #' @importFrom stats na.omit +#' @importFrom tools R_user_dir #' @importFrom utils capture.output getParseData getTxtProgressBar globalVariables head relist #' setTxtProgressBar tail txtProgressBar #' @importFrom xml2 as_list #' xml_attr xml_children xml_find_all xml_find_chr xml_find_lgl xml_find_num xml_find_first xml_name xml_text -#' @rawNamespace -#' if (getRversion() >= "4.0.0") { -#' importFrom(tools, R_user_dir) -#' } else { -#' importFrom(backports, R_user_dir) -#' } ## lintr namespace: end NULL diff --git a/R/utils.R b/R/utils.R index f6e709028..531ac1d89 100644 --- a/R/utils.R +++ b/R/utils.R @@ -198,10 +198,9 @@ platform_independent_sort <- function(x) x[platform_independent_order(x)] #' Extract text from `STR_CONST` nodes #' #' Convert `STR_CONST` `text()` values into R strings. This is useful to account for arbitrary -#' character literals valid since R 4.0, e.g. `R"------[hello]------"`, which is parsed in -#' R as `"hello"`. It is quite cumbersome to write XPaths allowing for strings like this, -#' so whenever your linter logic requires testing a `STR_CONST` node's value, use this -#' function. +#' character literals, e.g. `R"------[hello]------"`, which is parsed in R as `"hello"`. +#' It is quite cumbersome to write XPaths allowing for strings like this, so whenever your +#' linter logic requires testing a `STR_CONST` node's value, use this function. #' NB: this is also properly vectorized on `s`, and accepts a variety of inputs. Empty inputs #' will become `NA` outputs, which helps ensure that `length(get_r_string(s)) == length(s)`. #' @@ -219,15 +218,14 @@ platform_independent_sort <- function(x) x[platform_independent_order(x)] #' get_r_string(expr_as_xml, "expr[3]") #' unlink(tmp) #' -#' # more importantly, extract strings under R>=4 raw strings -#' @examplesIf getRversion() >= "4.0.0" -#' tmp4.0 <- tempfile() -#' writeLines("c(R'(a\\b)', R'--[a\\\"\'\"\\b]--')", tmp4.0) -#' expr_as_xml4.0 <- get_source_expressions(tmp4.0)$expressions[[1L]]$xml_parsed_content -#' writeLines(as.character(expr_as_xml4.0)) -#' get_r_string(expr_as_xml4.0, "expr[2]") -#' get_r_string(expr_as_xml4.0, "expr[3]") -#' unlink(tmp4.0) +#' # more importantly, extract raw strings correctly +#' tmp_raw <- tempfile() +#' writeLines("c(R'(a\\b)', R'--[a\\\"\'\"\\b]--')", tmp_raw) +#' expr_as_xml_raw <- get_source_expressions(tmp_raw)$expressions[[1L]]$xml_parsed_content +#' writeLines(as.character(expr_as_xml_raw)) +#' get_r_string(expr_as_xml_raw, "expr[2]") +#' get_r_string(expr_as_xml_raw, "expr[3]") +#' unlink(tmp_raw) #' #' @export get_r_string <- function(s, xpath = NULL) { diff --git a/R/zzz.R b/R/zzz.R index 291539297..b8d317d9f 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -295,9 +295,8 @@ settings <- new.env(parent = emptyenv()) toset <- !(names(op_lintr) %in% names(op)) if (any(toset)) options(op_lintr[toset]) - # R>=4.0.0: deparse1 # R>=4.1.0: ...names - backports::import(pkgname, c("deparse1", "...names")) + backports::import(pkgname, "...names") utils::assignInMyNamespace("default_settings", list( linters = default_linters, diff --git a/man/absolute_path_linter.Rd b/man/absolute_path_linter.Rd index f8b40a312..04fa1cf1a 100644 --- a/man/absolute_path_linter.Rd +++ b/man/absolute_path_linter.Rd @@ -18,9 +18,6 @@ If \code{TRUE}, only lint path strings, which Check that no absolute paths are used (e.g. "/var", "C:\\System", "~/docs"). } \examples{ -\dontshow{if (getRversion() >= "4.0") (if (getRversion() >= "3.4") withAutoprint else force)(\{ # examplesIf} -# Following examples use raw character constant syntax introduced in R 4.0. - # will produce lints lint( text = 'R"--[/blah/file.txt]--"', @@ -32,7 +29,7 @@ lint( text = 'R"(./blah)"', linters = absolute_path_linter() ) -\dontshow{\}) # examplesIf} + } \seealso{ \itemize{ diff --git a/man/get_r_string.Rd b/man/get_r_string.Rd index b1564c601..418d0f17c 100644 --- a/man/get_r_string.Rd +++ b/man/get_r_string.Rd @@ -15,10 +15,9 @@ and \code{xpath} is specified, it is extracted with \code{\link[xml2:xml_find_al } \description{ Convert \code{STR_CONST} \code{text()} values into R strings. This is useful to account for arbitrary -character literals valid since R 4.0, e.g. \code{R"------[hello]------"}, which is parsed in -R as \code{"hello"}. It is quite cumbersome to write XPaths allowing for strings like this, -so whenever your linter logic requires testing a \code{STR_CONST} node's value, use this -function. +character literals, e.g. \code{R"------[hello]------"}, which is parsed in R as \code{"hello"}. +It is quite cumbersome to write XPaths allowing for strings like this, so whenever your +linter logic requires testing a \code{STR_CONST} node's value, use this function. NB: this is also properly vectorized on \code{s}, and accepts a variety of inputs. Empty inputs will become \code{NA} outputs, which helps ensure that \code{length(get_r_string(s)) == length(s)}. } @@ -31,14 +30,13 @@ get_r_string(expr_as_xml, "expr[2]") get_r_string(expr_as_xml, "expr[3]") unlink(tmp) -# more importantly, extract strings under R>=4 raw strings -\dontshow{if (getRversion() >= "4.0.0") (if (getRversion() >= "3.4") withAutoprint else force)(\{ # examplesIf} -tmp4.0 <- tempfile() -writeLines("c(R'(a\\\\b)', R'--[a\\\\\"\'\"\\\\b]--')", tmp4.0) -expr_as_xml4.0 <- get_source_expressions(tmp4.0)$expressions[[1L]]$xml_parsed_content -writeLines(as.character(expr_as_xml4.0)) -get_r_string(expr_as_xml4.0, "expr[2]") -get_r_string(expr_as_xml4.0, "expr[3]") -unlink(tmp4.0) -\dontshow{\}) # examplesIf} +# more importantly, extract raw strings correctly +tmp_raw <- tempfile() +writeLines("c(R'(a\\\\b)', R'--[a\\\\\"\'\"\\\\b]--')", tmp_raw) +expr_as_xml_raw <- get_source_expressions(tmp_raw)$expressions[[1L]]$xml_parsed_content +writeLines(as.character(expr_as_xml_raw)) +get_r_string(expr_as_xml_raw, "expr[2]") +get_r_string(expr_as_xml_raw, "expr[3]") +unlink(tmp_raw) + } From 2a8a857a5cda87ce1209820d29e8b679639f75ec Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Fri, 14 Jun 2024 23:32:07 -0700 Subject: [PATCH 3/5] Assume stringsAsFactors=FALSE (#2605) * Assume stringsAsFactors=FALSE * disable corresponding linter * revert one change, as.data.frame.table keeps old sAF=TRUE * more call sites --- .lintr | 2 ++ R/get_source_expressions.R | 3 +-- R/linter_tags.R | 8 ++------ R/methods.R | 5 ++--- R/namespace.R | 12 ++++-------- R/object_overwrite_linter.R | 3 +-- R/shared_constants.R | 2 +- tests/testthat/test-fixed_regex_linter.R | 2 +- tests/testthat/test-linter_tags.R | 7 ++----- tests/testthat/test-methods.R | 5 ++--- tests/testthat/test-with.R | 7 +------ vignettes/lintr.Rmd | 8 ++------ 12 files changed, 21 insertions(+), 43 deletions(-) diff --git a/.lintr b/.lintr index 0542b9eee..0d4e1af9f 100644 --- a/.lintr +++ b/.lintr @@ -28,6 +28,8 @@ linters: all_linters( absolute_path_linter = NULL, library_call_linter = NULL, nonportable_path_linter = NULL, + # We now require R>=4.0.0 + strings_as_factors_linter = NULL, todo_comment_linter = NULL, # TODO(#2327): Enable this. unreachable_code_linter = NULL diff --git a/R/get_source_expressions.R b/R/get_source_expressions.R index bc83712d5..ec8a4d406 100644 --- a/R/get_source_expressions.R +++ b/R/get_source_expressions.R @@ -636,8 +636,7 @@ fix_eq_assigns <- function(pc) { parent = integer(n_expr), token = character(n_expr), terminal = logical(n_expr), - text = character(n_expr), - stringsAsFactors = FALSE + text = character(n_expr) ) for (i in seq_len(n_expr)) { diff --git a/R/linter_tags.R b/R/linter_tags.R index c8cce3fec..451ddd565 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -82,11 +82,7 @@ available_linters <- function(packages = "lintr", tags = NULL, exclude_tags = "d } build_available_linters <- function(available, package, tags, exclude_tags) { - available_df <- data.frame( - linter = available[["linter"]], - package, - stringsAsFactors = FALSE - ) + available_df <- data.frame(linter = available[["linter"]], package) available_df$tags <- strsplit(available[["tags"]], split = " ", fixed = TRUE) if (!is.null(tags)) { matches_tags <- vapply(available_df$tags, function(linter_tags) any(linter_tags %in% tags), logical(1L)) @@ -108,7 +104,7 @@ build_available_linters <- function(available, package, tags, exclude_tags) { #' `data.frame` constructors don't handle zero-row list-columns properly, so supply `tags` afterwards. #' @noRd empty_linters <- function() { - empty_df <- data.frame(linter = character(), package = character(), stringsAsFactors = FALSE) + empty_df <- data.frame(linter = character(), package = character()) empty_df$tags <- list() empty_df } diff --git a/R/methods.R b/R/methods.R index 4ff71c0a1..46e8ed804 100644 --- a/R/methods.R +++ b/R/methods.R @@ -163,8 +163,7 @@ as.data.frame.lints <- function(x, row.names = NULL, optional = FALSE, ...) { # type = vapply(x, `[[`, character(1L), "type"), message = vapply(x, `[[`, character(1L), "message"), line = vapply(x, `[[`, character(1L), "line"), - linter = vapply(x, `[[`, character(1L), "linter"), - stringsAsFactors = FALSE + linter = vapply(x, `[[`, character(1L), "linter") ) } @@ -198,7 +197,7 @@ summary.lints <- function(object, ...) { ) tbl <- table(filenames, types) filenames <- rownames(tbl) - res <- as.data.frame.matrix(tbl, stringsAsFactors = FALSE, row.names = NULL) + res <- as.data.frame.matrix(tbl, row.names = NULL) res$filenames <- filenames %||% character() nms <- colnames(res) res[order(res$filenames), c("filenames", nms[nms != "filenames"])] diff --git a/R/namespace.R b/R/namespace.R index a3fba3146..01ae56328 100644 --- a/R/namespace.R +++ b/R/namespace.R @@ -24,18 +24,18 @@ safe_get_exports <- function(ns) { # importFrom directives appear as list(ns, imported_funs) if (length(ns) > 1L) { - return(data.frame(pkg = ns[[1L]], fun = ns[[2L]], stringsAsFactors = FALSE)) + return(data.frame(pkg = ns[[1L]], fun = ns[[2L]])) } # relevant only if there are any exported objects fun <- getNamespaceExports(ns) if (length(fun) > 0L) { - data.frame(pkg = ns, fun = fun, stringsAsFactors = FALSE) + data.frame(pkg = ns, fun = fun) } } empty_namespace_data <- function() { - data.frame(pkg = character(), fun = character(), stringsAsFactors = FALSE) + data.frame(pkg = character(), fun = character()) } # filter namespace_imports() for S3 generics @@ -64,11 +64,7 @@ exported_s3_generics <- function(path = find_package(".")) { return(empty_namespace_data()) } - data.frame( - pkg = basename(path), - fun = unique(namespace_data$S3methods[, 1L]), - stringsAsFactors = FALSE - ) + data.frame(pkg = basename(path), fun = unique(namespace_data$S3methods[, 1L])) } is_s3_generic <- function(fun) { diff --git a/R/object_overwrite_linter.R b/R/object_overwrite_linter.R index 3c068503d..099129ed5 100644 --- a/R/object_overwrite_linter.R +++ b/R/object_overwrite_linter.R @@ -66,8 +66,7 @@ object_overwrite_linter <- function( ) pkg_exports <- data.frame( package = rep(packages, lengths(pkg_exports)), - name = unlist(pkg_exports), - stringsAsFactors = FALSE + name = unlist(pkg_exports) ) # Take the first among duplicate names, e.g. 'plot' resolves to base::plot, not graphics::plot diff --git a/R/shared_constants.R b/R/shared_constants.R index 5f1371584..3dc20d615 100644 --- a/R/shared_constants.R +++ b/R/shared_constants.R @@ -115,7 +115,7 @@ get_token_replacement <- function(token_content, token_type) { # r_string gives the operator as you would write it in R code. # styler: off -infix_metadata <- data.frame(stringsAsFactors = FALSE, matrix(byrow = TRUE, ncol = 2L, c( +infix_metadata <- data.frame(matrix(byrow = TRUE, ncol = 2L, c( "OP-PLUS", "+", "OP-MINUS", "-", "OP-TILDE", "~", diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R index 371848200..7f4075809 100644 --- a/tests/testthat/test-fixed_regex_linter.R +++ b/tests/testthat/test-fixed_regex_linter.R @@ -68,7 +68,7 @@ patrick::with_parameters_test_that( "|", "[", "]", "\\\\", "<", ">", "=", ":", ";", "/", "_", "-", "!", "@", "#", "%", "&", "~" ) - data.frame(char = char, .test_name = char, stringsAsFactors = FALSE) + data.frame(char = char, .test_name = char) }) ) diff --git a/tests/testthat/test-linter_tags.R b/tests/testthat/test-linter_tags.R index 985cdb30d..2d7f53810 100644 --- a/tests/testthat/test-linter_tags.R +++ b/tests/testthat/test-linter_tags.R @@ -13,11 +13,7 @@ test_that("validate_linter_db works as expected", { ) expect_false(suppressWarnings(lintr:::validate_linter_db(df_empty, "mypkg"))) - df <- data.frame( - linter = "absolute_path_linter", - tags = "robustness", - stringsAsFactors = FALSE - ) + df <- data.frame(linter = "absolute_path_linter", tags = "robustness") expect_true(lintr:::validate_linter_db(df, "mypkg")) }) @@ -160,6 +156,7 @@ test_that("lintr help files are up to date", { ) # Counts of tags from available_linters() + # NB: as.data.frame.table returns stringsAsFactors=TRUE default in R>4 db_tag_table <- as.data.frame( table(tag = unlist(lintr_db$tags)), responseName = "n_linters", diff --git a/tests/testthat/test-methods.R b/tests/testthat/test-methods.R index 9f454c1f4..172b9692d 100644 --- a/tests/testthat/test-methods.R +++ b/tests/testthat/test-methods.R @@ -56,14 +56,13 @@ test_that("as.data.frame.lints", { expect_s3_class(df, "data.frame") exp <- data.frame( - filename = rep("dummy.R", 2L), + filename = "dummy.R", line_number = c(1L, 2L), column_number = c(1L, 6L), type = c("style", "error"), message = c("", "Under no circumstances is the use of foobar allowed."), line = c("", "a <- 1"), - linter = c(NA_character_, NA_character_), # These are assigned in lint() now. - stringsAsFactors = FALSE + linter = NA_character_ # These are assigned in lint() now. ) expect_identical(df, exp) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 5ab2560ad..fff11bced 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -24,12 +24,7 @@ test_that("linters_with_defaults warns on unused NULLs", { test_that("linters_with_tags() verifies the output of available_linters()", { local_mocked_bindings( available_linters = function(...) { - data.frame( - linter = c("fake_linter", "very_fake_linter"), - package = "lintr", - tags = "", - stringsAsFactors = FALSE - ) + data.frame(linter = c("fake_linter", "very_fake_linter"), package = "lintr", tags = "") } ) expect_error( diff --git a/vignettes/lintr.Rmd b/vignettes/lintr.Rmd index e6c4a1da0..b819c9a12 100644 --- a/vignettes/lintr.Rmd +++ b/vignettes/lintr.Rmd @@ -140,10 +140,7 @@ make_string <- function(x) { } } -defaults_table <- data.frame( - default = vapply(default_settings, make_string, character(1L)), - stringsAsFactors = FALSE -) +defaults_table <- data.frame(default = vapply(default_settings, make_string, character(1L))) # avoid conflict when loading lintr in echo=TRUE cell below rm(default_settings) @@ -196,8 +193,7 @@ make_setting_string <- function(linter_name) { defaults_table <- data.frame( row.names = names(default_linters), - settings = vapply(names(default_linters), make_setting_string, character(1L)), - stringsAsFactors = FALSE + settings = vapply(names(default_linters), make_setting_string, character(1L)) ) knitr::kable(defaults_table) From b96c8475a0a549ec97a47813efd10d24f57c058b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 15 Jun 2024 01:42:58 -0700 Subject: [PATCH 4/5] Use raw strings in some places (#2604) --- R/zzz.R | 8 +- tests/testthat/test-fixed_regex_linter.R | 168 +++++++++++------------ 2 files changed, 85 insertions(+), 91 deletions(-) diff --git a/R/zzz.R b/R/zzz.R index b8d317d9f..e281353db 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -168,9 +168,9 @@ default_undesirable_functions <- all_undesirable_functions[names(all_undesirable rd_auto_link <- function(x) { x <- unlist(x) - x <- gsub("([a-zA-Z0-9.]+)::([a-zA-Z0-9._]+)\\(\\)", "\\\\code{\\\\link[\\1:\\2]{\\1::\\2()}}", x) - x <- gsub("([^:a-zA-Z0-9._])([a-zA-Z0-9._]+)\\(\\)", "\\1\\\\code{\\\\link[=\\2]{\\2()}}", x) - x <- gsub("`([^`]+)`", "\\\\code{\\1}", x) + x <- gsub(R"{([a-zA-Z0-9.]+)::([a-zA-Z0-9._]+)\(\)}", R"(\\code{\\link[\1:\2]{\1::\2()}})", x) + x <- gsub(R"{([^:a-zA-Z0-9._])([a-zA-Z0-9._]+)\(\)}", R"(\1\\code{\\link[=\2]{\2()}})", x) + x <- gsub("`([^`]+)`", R"(\\code{\1})", x) x } @@ -181,7 +181,7 @@ rd_undesirable_functions <- function() { "The following functions are sometimes regarded as undesirable:", "\\itemize{", sprintf( - "\\item \\code{\\link[=%1$s]{%1$s()}} As an alternative, %2$s.", + R"(\item \code{\link[=%1$s]{%1$s()}} As an alternative, %2$s.)", names(default_undesirable_functions), alternatives ), "}" diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R index 7f4075809..98a279a02 100644 --- a/tests/testthat/test-fixed_regex_linter.R +++ b/tests/testthat/test-fixed_regex_linter.R @@ -1,9 +1,3 @@ -# NB: escaping is confusing. We have to double-escape everything -- the first -# escape creates a string that will be parse()d, the second escape is normal -# escaping that would be done in R code. E.g. in "\\\\.", the R code would -# read like "\\.", but in order to create those two slashes, we need to write -# "\\\\." in the string here. - test_that("fixed_regex_linter skips allowed usages", { linter <- fixed_regex_linter() @@ -11,15 +5,15 @@ test_that("fixed_regex_linter skips allowed usages", { expect_lint("grep('x$', '', y)", NULL, linter) expect_lint("sub('[a-zA-Z]', '', y)", NULL, linter) expect_lint("grepl(fmt, y)", NULL, linter) - expect_lint("regexec('\\\\s', '', y)", NULL, linter) + expect_lint(R"{regexec('\\s', '', y)}", NULL, linter) expect_lint("grep('a(?=b)', x, perl = TRUE)", NULL, linter) expect_lint("grep('0+1', x, perl = TRUE)", NULL, linter) expect_lint("grep('1*2', x)", NULL, linter) expect_lint("grep('a|b', x)", NULL, linter) - expect_lint("grep('\\\\[|\\\\]', x)", NULL, linter) + expect_lint(R"{grep('\\[|\\]', x)}", NULL, linter) # if fixed=TRUE is already set, regex patterns don't matter - expect_lint("gsub('\\\\.', '', y, fixed = TRUE)", NULL, linter) + expect_lint(R"{gsub('\\.', '', y, fixed = TRUE)}", NULL, linter) # ignore.case=TRUE implies regex interpretation expect_lint("gsub('abcdefg', '', y, ignore.case = TRUE)", NULL, linter) @@ -37,10 +31,10 @@ test_that("fixed_regex_linter blocks simple disallowed usages", { linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("gsub('\\\\.', '', x)", lint_msg, linter) + expect_lint(R"{gsub('\\.', '', x)}", lint_msg, linter) expect_lint("grepl('abcdefg', x)", lint_msg, linter) expect_lint("gregexpr('a-z', y)", lint_msg, linter) - expect_lint("regexec('\\\\$', x)", lint_msg, linter) + expect_lint(R"{regexec('\\$', x)}", lint_msg, linter) expect_lint("grep('\n', x)", lint_msg, linter) # naming the argument doesn't matter (if it's still used positionally) @@ -51,13 +45,13 @@ patrick::with_parameters_test_that( "fixed_regex_linter is robust to unrecognized escapes error", { expect_lint( - sprintf("grep('\\\\%s', x)", char), + sprintf(R"{grep('\\%s', x)}", char), rex::rex("This regular expression is static"), fixed_regex_linter() ) expect_lint( - sprintf("strsplit('a%sb', '\\\\%s')", char, char), + sprintf(R"{strsplit('a%sb', '\\%s')}", char, char), rex::rex("This regular expression is static"), fixed_regex_linter() ) @@ -65,7 +59,7 @@ patrick::with_parameters_test_that( .cases = local({ char <- c( "^", "$", "{", "}", "(", ")", ".", "*", "+", "?", - "|", "[", "]", "\\\\", "<", ">", "=", ":", ";", "/", + "|", "[", "]", R"(\\)", "<", ">", "=", ":", ";", "/", "_", "-", "!", "@", "#", "%", "&", "~" ) data.frame(char = char, .test_name = char) @@ -87,7 +81,7 @@ test_that("fixed_regex_linter catches null calls to strsplit as well", { linter <- fixed_regex_linter() expect_lint("strsplit(x, '^x')", NULL, linter) - expect_lint("strsplit(x, '\\\\s')", NULL, linter) + expect_lint(R"{strsplit(x, '\\s')}", NULL, linter) expect_lint("strsplit(x, 'a(?=b)', perl = TRUE)", NULL, linter) expect_lint("strsplit(x, '0+1', perl = TRUE)", NULL, linter) expect_lint("strsplit(x, 'a|b')", NULL, linter) @@ -97,15 +91,15 @@ test_that("fixed_regex_linter catches null calls to strsplit as well", { expect_lint("tstrsplit(x, fmt)", NULL, linter) # if fixed=TRUE is already set, regex patterns don't matter - expect_lint("strsplit(x, '\\\\.', fixed = TRUE)", NULL, linter) - expect_lint("strsplit(x, '\\\\.', fixed = T)", NULL, linter) + expect_lint(R"{strsplit(x, '\\.', fixed = TRUE)}", NULL, linter) + expect_lint(R"{strsplit(x, '\\.', fixed = T)}", NULL, linter) }) test_that("fixed_regex_linter catches calls to strsplit as well", { linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("strsplit(x, '\\\\.')", lint_msg, linter) + expect_lint(R"{strsplit(x, '\\.')}", lint_msg, linter) expect_lint("strsplit(x, '[.]')", lint_msg, linter) expect_lint("tstrsplit(x, 'abcdefg')", lint_msg, linter) @@ -115,8 +109,8 @@ test_that("fixed_regex_linter is more exact about distinguishing \\s from \\:", linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("grep('\\\\s', '', x)", NULL, linter) - expect_lint("grep('\\\\:', '', x)", lint_msg, linter) + expect_lint(R"{grep('\\s', '', x)}", NULL, linter) + expect_lint(R"{grep('\\:', '', x)}", lint_msg, linter) }) ## tests for stringr functions @@ -126,12 +120,12 @@ test_that("fixed_regex_linter skips allowed stringr usages", { expect_lint("str_replace(y, '[a-zA-Z]', '')", NULL, linter) expect_lint("str_replace_all(y, '^x', '')", NULL, linter) expect_lint("str_detect(y, fmt)", NULL, linter) - expect_lint("str_extract(y, '\\\\s')", NULL, linter) - expect_lint("str_extract_all(y, '\\\\s')", NULL, linter) + expect_lint(R"{str_extract(y, '\\s')}", NULL, linter) + expect_lint(R"{str_extract_all(y, '\\s')}", NULL, linter) expect_lint("str_which(x, '1*2')", NULL, linter) # if fixed() is already set, regex patterns don't matter - expect_lint("str_replace(y, fixed('\\\\.'), '')", NULL, linter) + expect_lint(R"{str_replace(y, fixed('\\.'), '')}", NULL, linter) # namespace qualification doesn't matter expect_lint("stringr::str_replace(y, stringr::fixed('abcdefg'), '')", NULL, linter) @@ -141,16 +135,16 @@ test_that("fixed_regex_linter blocks simple disallowed usages of stringr functio linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("str_replace_all(x, '\\\\.', '')", lint_msg, linter) + expect_lint(R"{str_replace_all(x, '\\.', '')}", lint_msg, linter) expect_lint("str_detect(x, 'abcdefg')", lint_msg, linter) expect_lint("str_locate(y, 'a-z')", lint_msg, linter) - expect_lint("str_subset(x, '\\\\$')", lint_msg, linter) + expect_lint(R"{str_subset(x, '\\$')}", lint_msg, linter) expect_lint("str_which(x, '\n')", lint_msg, linter) # named, positional arguments are still caught expect_lint("str_locate(y, pattern = 'a-z')", lint_msg, linter) # nor do other named arguments throw things off - expect_lint("str_starts(x, '\\\\.', negate = TRUE)", lint_msg, linter) + expect_lint(R"{str_starts(x, '\\.', negate = TRUE)}", lint_msg, linter) }) test_that("fixed_regex_linter catches calls to str_split as well", { @@ -161,8 +155,8 @@ test_that("fixed_regex_linter catches calls to str_split as well", { expect_lint("str_split(x, fmt)", NULL, linter) # if fixed() is already set, regex patterns don't matter - expect_lint("str_split(x, fixed('\\\\.'))", NULL, linter) - expect_lint("str_split(x, '\\\\.')", lint_msg, linter) + expect_lint(R"{str_split(x, fixed('\\.'))}", NULL, linter) + expect_lint(R"{str_split(x, '\\.')}", lint_msg, linter) expect_lint("str_split(x, '[.]')", lint_msg, linter) }) @@ -187,24 +181,24 @@ test_that("one-character character classes with escaped characters are caught", linter <- fixed_regex_linter() lint_msg <- rex::rex("This regular expression is static") - expect_lint("gsub('[\\n]', '', x)", lint_msg, linter) - expect_lint("gsub('[\\\"]', '', x)", lint_msg, linter) - expect_lint('gsub("\\\\<", "x", x, perl = TRUE)', lint_msg, linter) - - expect_lint("str_split(x, '[\\1]')", lint_msg, linter) - expect_lint("str_split(x, '[\\12]')", lint_msg, linter) - expect_lint("str_split(x, '[\\123]')", lint_msg, linter) - expect_lint("str_split(x, '[\\xa]')", lint_msg, linter) - expect_lint("str_split(x, '[\\x32]')", lint_msg, linter) - expect_lint("str_split(x, '[\\uF]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u01]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u012]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u0123]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U8]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U1d4d7]')", lint_msg, linter) - expect_lint("str_split(x, '[\\u{1}]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U{F7D5}]')", lint_msg, linter) - expect_lint("str_split(x, '[\\U{1D4D7}]')", lint_msg, linter) + expect_lint(R"{gsub('[\n]', '', x)}", lint_msg, linter) + expect_lint(R"{gsub('[\"]', '', x)}", lint_msg, linter) + expect_lint(R'{gsub("\\<", "x", x, perl = TRUE)}', lint_msg, linter) + + expect_lint(R"{str_split(x, '[\1]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\12]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\123]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\xa]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\x32]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\uF]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u01]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u012]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u0123]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U8]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U1d4d7]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\u{1}]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U{F7D5}]')}", lint_msg, linter) + expect_lint(R"{str_split(x, '[\U{1D4D7}]')}", lint_msg, linter) }) test_that("bracketed unicode escapes are caught", { @@ -218,15 +212,15 @@ test_that("bracketed unicode escapes are caught", { test_that("escaped characters are handled correctly", { linter <- fixed_regex_linter() - expect_lint("gsub('\\n+', '', sql)", NULL, linter) + expect_lint(R"{gsub('\n+', '', sql)}", NULL, linter) expect_lint('gsub("\\n{2,}", "\n", D)', NULL, linter) - expect_lint('gsub("[\\r\\n]", "", x)', NULL, linter) - expect_lint('gsub("\\n $", "", y)', NULL, linter) - expect_lint('gsub("```\\n*```r*\\n*", "", x)', NULL, linter) + expect_lint(R'{gsub("[\r\n]", "", x)}', NULL, linter) + expect_lint(R'{gsub("\n $", "", y)}', NULL, linter) + expect_lint(R'{gsub("```\n*```r*\n*", "", x)}', NULL, linter) expect_lint('strsplit(x, "(;|\n)")', NULL, linter) - expect_lint('strsplit(x, "(;|\\n)")', NULL, linter) - expect_lint('grepl("[\\\\W]", x, perl = TRUE)', NULL, linter) - expect_lint('grepl("[\\\\W]", x)', NULL, linter) + expect_lint(R'{strsplit(x, "(;|\n)")}', NULL, linter) + expect_lint(R'{grepl("[\\W]", x, perl = TRUE)}', NULL, linter) + expect_lint(R'{grepl("[\\W]", x)}', NULL, linter) }) # make sure the logic is properly vectorized @@ -238,10 +232,10 @@ test_that("fixed replacements vectorize and recognize str_detect", { linter <- fixed_regex_linter() # properly vectorized expect_lint( - trim_some("{ + trim_some(R"({ grepl('abcdefg', x) - grepl('a[.]\\\\.b\\n', x) - }"), + grepl('a[.]\\.b\n', x) + })"), list( list(rex::rex('Use "abcdefg" with fixed = TRUE'), line_number = 2L), list(rex::rex('Use "a..b\\n" with fixed = TRUE'), line_number = 3L) @@ -289,37 +283,37 @@ robust_non_printable_unicode <- function() { # styler: off local({ .cases <- tibble::tribble( - ~.test_name, ~regex_expr, ~fixed_expr, - "[.]", "[.]", ".", - '[\\\"]', '[\\\"]', '\\"', - "[]]", "[]]", "]", - "\\\\.", "\\\\.", ".", - "\\\\:", "\\\\:", ":", - "\\\\<", "\\\\<", "<", - "\\\\$", "\\\\$", "$", - "[\\1]", "[\\1]", "\\001", - "\\1", "\\1", "\\001", - "[\\12]", "[\\12]", "\\n", - "[\\123]", "[\\123]", "S", - "a[*]b", "a[*]b", "a*b", - "abcdefg", "abcdefg", "abcdefg", - "abc\\U{A0DEF}ghi", "abc\\U{A0DEF}ghi", robust_non_printable_unicode(), - "a-z", "a-z", "a-z", - "[\\n]", "[\\n]", "\\n", - "\\n", "\n", "\\n", - "[\\u01]", "[\\u01]", "\\001", - "[\\u012]", "[\\u012]", "\\022", - "[\\u0123]", "[\\u0123]", "\u0123", - "[\\u{1}]", "[\\u{1}]", "\\001", - "[\\U1d4d7]", "[\\U1d4d7]", "\U1D4D7", - "[\\U{1D4D7}]", "[\\U{1D4D7}]", "\U1D4D7", - "[\\U8]", "[\\U8]", "\\b", - "\\u{A0}", "\\u{A0}", "\uA0", - "\\u{A0}\\U{0001d4d7}", "\\u{A0}\\U{0001d4d7}", "\uA0\U1D4D7", - "[\\uF]", "[\\uF]", "\\017", - "[\\U{F7D5}]", "[\\U{F7D5}]", "\UF7D5", - "[\\x32]", "[\\x32]", "2", - "[\\xa]", "[\\xa]", "\\n" + ~.test_name, ~regex_expr, ~fixed_expr, + "[.]", "[.]", ".", + '[\\\"]', R'([\"])', '\\"', + "[]]", "[]]", "]", + R"(\\.)", R"(\\.)", ".", + R"(\\:)", R"(\\:)", ":", + R"(\\<)", R"(\\<)", "<", + R"(\\$)", R"(\\$)", "$", + R"([\1])", R"([\1])", R"(\001)", + R"(\1)", R"(\1)", R"(\001)", + R"([\12])", R"([\12])", R"(\n)", + R"([\123])", R"([\123])", "S", + "a[*]b", "a[*]b", "a*b", + "abcdefg", "abcdefg", "abcdefg", + "abc\\U{A0DEF}ghi", "abc\\U{A0DEF}ghi", robust_non_printable_unicode(), + "a-z", "a-z", "a-z", + R"([\n])", R"([\n])", R"(\n)", + R"(\n)", "\n", R"(\n)", + R"([\u01])", R"([\u01])", R"(\001)", + R"([\u012])", R"([\u012])", R"(\022)", + R"([\u0123])", R"([\u0123])", "\u0123", + R"([\u{1}])", R"([\u{1}])", R"(\001)", + R"([\U1d4d7])", R"([\U1d4d7])", "\U1D4D7", + R"([\U{1D4D7}])", R"([\U{1D4D7}])", "\U1D4D7", + R"([\U8])", R"([\U8])", R"(\b)", + R"(\u{A0})", R"(\u{A0})", "\uA0", + R"(\u{A0}\U{0001d4d7})", R"(\u{A0}\U{0001d4d7})", "\uA0\U1D4D7", + R"([\uF])", R"([\uF])", R"(\017)", + R"([\U{F7D5}])", R"([\U{F7D5}])", "\UF7D5", + R"([\x32])", R"([\x32])", "2", + R"([\xa])", R"([\xa])", R"(\n)" ) if (.Platform$OS.type == "windows" && !hasName(R.Version(), "crt")) { skip_cases <- c( From 10de2ae2f91a346671fc5a32938e0f038689012f Mon Sep 17 00:00:00 2001 From: olivroy <52606734+olivroy@users.noreply.github.com> Date: Mon, 17 Jun 2024 09:55:05 -0400 Subject: [PATCH 5/5] Use cli error chaining + add clickable link to bad config (#2602) --- R/cache.R | 8 ++-- R/lint.R | 42 +++++++++--------- R/settings.R | 71 ++++++++++++++++++------------ R/with.R | 28 +++++++----- R/xml_nodes_to_lints.R | 2 +- man/read_settings.Rd | 4 +- tests/testthat/test-Lint-builder.R | 10 ++--- tests/testthat/test-lint_package.R | 4 +- tests/testthat/test-with.R | 17 ++++--- 9 files changed, 108 insertions(+), 78 deletions(-) diff --git a/R/cache.R b/R/cache.R index e2a63bb4b..b177f4305 100644 --- a/R/cache.R +++ b/R/cache.R @@ -53,10 +53,10 @@ load_cache <- function(file, path = NULL) { invokeRestart("muffleWarning") }, error = function(e) { - cli_warn(c( - x = "Could not load cache file {.file {file}}:", - i = conditionMessage(e) - )) + cli_warn( + "Could not load cache file {.file {file}}:", + parent = e + ) } ) } # else nothing to do for source file that has no cache diff --git a/R/lint.R b/R/lint.R index 48ae96e81..61dc95254 100644 --- a/R/lint.R +++ b/R/lint.R @@ -88,10 +88,10 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = lints[[length(lints) + 1L]] <- withCallingHandlers( get_lints(expr, linter, linters[[linter]], lint_cache, source_expressions$lines), error = function(cond) { - cli_abort(c( + cli_abort( "Linter {.fn linter} failed in {.file {filename}}:", - conditionMessage(cond) - )) + parent = cond + ) } ) } @@ -410,16 +410,13 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec } max_col <- max(nchar(line) + 1L, 1L, na.rm = TRUE) if (!is_number(column_number) || column_number < 0L || column_number > max_col) { - cli_abort(c( - i = "{.arg column_number} must be an integer between {.val {0}} and {.code nchar(line) + 1} ({.val {max_col}})", - x = "Instead, it was {.val {column_number}}." - )) + cli_abort(" + {.arg column_number} must be an integer between {.val {0}} and {.val {max_col}} ({.code nchar(line) + 1}), + not {.obj_type_friendly {column_number}}. + ") } if (!is_number(line_number) || line_number < 1L) { - cli_abort(c( - i = "{.arg line_number} must be a positive integer.", - x = "Instead, it was {.val {line_number}}." - )) + cli_abort("{.arg line_number} must be a positive integer, not {.obj_type_friendly {line_number}}.") } check_ranges(ranges, max_col) @@ -449,25 +446,28 @@ is_valid_range <- function(range, max_col) { range[[2L]] <= max_col } -check_ranges <- function(ranges, max_col) { +check_ranges <- function(ranges, max_col, call = parent.frame()) { if (is.null(ranges)) { return() } if (!is.list(ranges)) { - cli_abort(c( - i = "{.arg ranges} must be {.code NULL} or a {.cls list}.", - x = "Instead, it was {.cls {class(ranges)}}." - )) + cli_abort( + "{.arg ranges} must be {.code NULL} or a list, not {.obj_type_friendly {ranges}}.", + call = call + ) } for (range in ranges) { if (!is_number(range, 2L)) { - cli_abort("{.arg ranges} must only contain {.cls integer} vectors of length 2 without {.code NA}s.") + cli_abort( + "{.arg ranges} must only contain integer vectors of length 2 without {.code NA}s.", + call = call + ) } else if (!is_valid_range(range, max_col)) { - cli_abort(c( - x = "Invalid range specified.", - i = "Argument {.arg ranges} must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 ({.val {max_col}})." - )) + cli_abort( + "{.arg ranges} must satisfy {.val {0}} <= range[1L] <= range[2L] <= {.val {max_col}} (nchar(line) + 1).", + call = call + ) } } } diff --git a/R/settings.R b/R/settings.R index 614d19767..9bd7da159 100644 --- a/R/settings.R +++ b/R/settings.R @@ -61,7 +61,8 @@ #' ``` #' #' @param filename Source file to be linted. -read_settings <- function(filename) { +#' @param call Passed to malformed to ensure linear trace. +read_settings <- function(filename, call = parent.frame()) { reset_settings() config_file <- find_config(filename) @@ -71,7 +72,7 @@ read_settings <- function(filename) { default_settings[["encoding"]] <- default_encoding } - config <- read_config_file(config_file) + config <- read_config_file(config_file, call = call) validate_config_file(config, config_file, default_settings) for (setting in names(default_settings)) { @@ -89,48 +90,53 @@ read_settings <- function(filename) { } } -read_config_file <- function(config_file) { +#' @param call Passed to malformed to ensure linear trace. +#' @noRd +read_config_file <- function(config_file, call = parent.frame()) { if (is.null(config_file)) { return(NULL) } + # clickable link for eventual error messages. + malformed_file <- link_config_file(config_file) # nolint: object_usage_linter. TODO(#2252). config <- new.env() if (endsWith(config_file, ".R")) { load_config <- function(file) sys.source(file, config, keep.source = FALSE, keep.parse.data = FALSE) malformed <- function(e) { - cli_abort(c( - "Malformed config file, ensure it is valid R syntax.", - conditionMessage(e) - )) + cli_abort( + "Malformed config file ({malformed_file}), ensure it is valid R syntax.", + parent = e, + call = call + ) } } else { load_config <- function(file) { dcf_values <- read.dcf(file, all = TRUE) for (setting in names(dcf_values)) { - parsed_setting <- tryCatch( + parsed_setting <- withCallingHandlers( str2lang(dcf_values[[setting]]), error = function(e) { - cli_abort(c( + cli_abort( "Malformed config setting {.field {setting}}:", - conditionMessage(e) - )) + parent = e + ) } ) setting_value <- withCallingHandlers( tryCatch( eval(parsed_setting), error = function(e) { - cli_abort(c( - "Error from config setting {.code {setting}} in {.code {format(conditionCall(e))}}:", - conditionMessage(e) - )) + cli_abort( + "Error from config setting {.code {setting}}.", + parent = e + ) } ), warning = function(w) { - cli_warn(c( - "Warning from config setting {.code {setting}} in {.code {format(conditionCall(w))}}:", - conditionMessage(w) - )) + cli_warn( + "Warning from config setting {.code {setting}}.", + parent = w + ) invokeRestart("muffleWarning") } ) @@ -138,10 +144,11 @@ read_config_file <- function(config_file) { } } malformed <- function(e) { - cli_abort(c( - x = "Malformed config file:", - i = conditionMessage(e) - )) + cli_abort( + "Malformed config file ({malformed_file}):", + parent = e, + call = call + ) } } withCallingHandlers( @@ -150,10 +157,10 @@ read_config_file <- function(config_file) { error = malformed ), warning = function(w) { - cli::cli_warn(c( - x = "Warning encountered while loading config:", - i = conditionMessage(w) - )) + cli::cli_warn( + "Warning encountered while loading config:", + parent = w + ) invokeRestart("muffleWarning") } ) @@ -164,7 +171,8 @@ validate_config_file <- function(config, config_file, defaults) { matched <- names(config) %in% names(defaults) if (!all(matched)) { unused_settings <- names(config)[!matched] # nolint: object_usage_linter. TODO(#2252). - cli_warn("Found unused settings in config {.str config_file}: {.field unused_settings}") + config_link <- link_config_file(config_file) # nolint: object_usage_linter. TODO(#2252). + cli_warn("Found unused settings in config file ({config_link}): {.field unused_settings}") } validate_regex(config, @@ -297,3 +305,10 @@ get_encoding_from_dcf <- function(file) { NULL } + +link_config_file <- function(path) { + cli::style_hyperlink( + cli::col_blue(basename(path)), + paste0("file://", path) + ) +} diff --git a/R/with.R b/R/with.R index f627f00ea..16d34fc7b 100644 --- a/R/with.R +++ b/R/with.R @@ -31,8 +31,11 @@ #' names(my_undesirable_functions) #' @export modify_defaults <- function(defaults, ...) { - if (missing(defaults) || !is.list(defaults) || !all(nzchar(names2(defaults)))) { - cli_abort("{.arg defaults} must be a named list.") + if (missing(defaults)) { + cli_abort("{.arg defaults} is a required argument, but is missing.") + } + if (!is.list(defaults) || !all(nzchar(names2(defaults)))) { + cli_abort("{.arg defaults} must be a named list, not {.obj_type_friendly {defaults}}.") } vals <- list(...) nms <- names2(vals) @@ -90,7 +93,7 @@ modify_defaults <- function(defaults, ...) { #' @export linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "deprecated") { if (!is.character(tags) && !is.null(tags)) { - cli_abort("{.arg tags} must be a {.cls character} vector, or {.code NULL}.") + cli_abort("{.arg tags} must be a character vector, or {.code NULL}, not {.obj_type_friendly {tags}}.") } tagged_linters <- list() @@ -102,8 +105,8 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep if (!all(available$linter %in% ns_exports)) { missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. TODO(#2252). cli_abort(c( - i = "Linters {.fn {missing_linters}} are advertised by {.fn available_linters}.", - x = "But these linters are not exported by package {.pkg {package}}." + x = "Can't find linters {.fn {missing_linters}}.", + i = "These are advertised by {.fn available_linters}, but are not exported by package {.pkg {package}}." )) } linter_factories <- mget(available$linter, envir = pkg_ns) @@ -178,9 +181,12 @@ linters_with_defaults <- function(..., defaults = default_linters) { dots <- list(...) if (missing(defaults) && "default" %in% names(dots)) { cli_warn(c( - "Did you mean {.arg defaults}?", - x = "{.arg default} is not an argument to {.fun linters_with_defaults}.", - i = "This warning will be removed when {.fun with_defaults} is fully deprecated." + x = " + {.arg default} is not an argument to {.help [{.fn linters_with_defaults}](lintr::linters_with_defaults)}. + ", + i = "Did you mean {.arg defaults}?", + # make message more subtle + cli::col_silver("This warning will be removed when {.fun with_defaults} is fully deprecated.") )) defaults <- dots$default nms <- names2(dots) @@ -207,10 +213,10 @@ call_linter_factory <- function(linter_factory, linter_name, package) { linter <- tryCatch( linter_factory(), error = function(e) { - cli_abort(c( + cli_abort( "Could not create linter with {.fun {package}::{linter_name}}.", - conditionMessage(e) - )) + parent = e + ) } ) # Otherwise, all linters would be called "linter_factory". diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index 9da6c450d..a893662bb 100644 --- a/R/xml_nodes_to_lints.R +++ b/R/xml_nodes_to_lints.R @@ -55,7 +55,7 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, } else if (!is_node(xml)) { cli_abort(c( x = "Expected an {.cls xml_nodeset}, a {.cls list} of xml_nodes, or an {.cls xml_node}.", - i = "Instead got an object of class(es): {.cls {class(xml)}}" + i = "Instead got {.obj_type_friendly {xml}}." )) } type <- match.arg(type, c("style", "warning", "error")) diff --git a/man/read_settings.Rd b/man/read_settings.Rd index bc02121c1..cbb1dc9ec 100644 --- a/man/read_settings.Rd +++ b/man/read_settings.Rd @@ -4,10 +4,12 @@ \alias{read_settings} \title{Read lintr settings} \usage{ -read_settings(filename) +read_settings(filename, call = parent.frame()) } \arguments{ \item{filename}{Source file to be linted.} + +\item{call}{Passed to malformed to ensure linear trace.} } \description{ Lintr searches for settings for a given source file in the following order: diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index d21c33f22..be5481f7b 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -2,7 +2,7 @@ test_that("Lint() errors on invalid input", { dummy_line <- "abc" expect_error( Lint("dummy.R", line = dummy_line, column_number = NA_integer_), - "`column_number` must be an integer between 0 and `nchar(line) + 1` (4)", + "`column_number` must be an integer between 0 and 4 (`nchar(line) + 1`)", fixed = TRUE ) expect_error( @@ -12,22 +12,22 @@ test_that("Lint() errors on invalid input", { ) expect_error( Lint("dummy.R", ranges = c(1L, 3L)), - "`ranges` must be `NULL` or a ", + "`ranges` must be `NULL` or a list", fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(1L)), - "`ranges` must only contain vectors of length 2 without `NA`s.", + "`ranges` must only contain integer vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(c(1L, NA_integer_))), - "`ranges` must only contain vectors of length 2 without `NA`s.", + "`ranges` must only contain integer vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( Lint("dummy.R", line = dummy_line, ranges = list(c(1L, 2L), c(1L, 5L))), - "Argument `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).", + "`ranges` must satisfy 0 <= range[1L] <= range[2L] <= 4 (nchar(line) + 1).", fixed = TRUE ) }) diff --git a/tests/testthat/test-lint_package.R b/tests/testthat/test-lint_package.R index 957b8af9c..b6c9b8b53 100644 --- a/tests/testthat/test-lint_package.R +++ b/tests/testthat/test-lint_package.R @@ -211,7 +211,7 @@ test_that("package using .lintr.R config lints correctly", { # config has bad R syntax expect_error( lint_package(test_path("dummy_packages", "RConfigInvalid")), - "Malformed config file, ensure it is valid R syntax", + "Malformed config file (lintr_test_config.R), ensure it is valid R syntax", fixed = TRUE ) @@ -219,7 +219,7 @@ test_that("package using .lintr.R config lints correctly", { withr::local_options(lintr.linter_file = "lintr_test_config_extraneous") expect_warning( expect_length(lint_package(r_config_pkg), 2L), - "Found unused settings in config", + "Found unused settings in config file", fixed = TRUE ) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index fff11bced..9aa49d204 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -1,11 +1,18 @@ test_that("modify_defaults produces error with missing or incorrect defaults", { - lint_msg <- "`defaults` must be a named list." - expect_error(modify_defaults(), lint_msg, fixed = TRUE) - expect_error(modify_defaults("assignment_linter"), lint_msg, fixed = TRUE) + expect_error( + modify_defaults(), + "`defaults` is a required argument, but is missing", + fixed = TRUE + ) + expect_error( + modify_defaults("assignment_linter"), + "`defaults` must be a named list", + fixed = TRUE + ) }) test_that("linters_with_tags produces error with incorrect tags", { - expect_error(linters_with_tags(1L:4L), "`tags` must be a vector, or `NULL`.", fixed = TRUE) + expect_error(linters_with_tags(1L:4L), "`tags` must be a character vector, or `NULL`", fixed = TRUE) }) test_that("linters_with_defaults works as expected with unnamed args", { @@ -29,7 +36,7 @@ test_that("linters_with_tags() verifies the output of available_linters()", { ) expect_error( linters_with_tags(NULL), - "Linters `fake_linter()` and `very_fake_linter()` are advertised by `available_linters()`", + "Can't find linters `fake_linter()` and `very_fake_linter()`", fixed = TRUE ) })