From a680b56f37cb9e80ea14eae19458b6d46f6f6567 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 11 Dec 2023 08:14:37 +0100 Subject: [PATCH 01/65] start with backport linter --- DESCRIPTION | 1 + R/backport_linter.R | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index ffa56a981..3eb5a0dcf 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -24,6 +24,7 @@ Depends: R (>= 3.6) Imports: backports (>= 1.1.7), + cli, codetools, cyclocomp, digest, diff --git a/R/backport_linter.R b/R/backport_linter.R index 8c46023fe..a8ed81f12 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -90,7 +90,7 @@ normalize_r_version <- function(r_version) { version_names <- c("devel", "release", paste0("oldrel-", seq_len(length(minor_versions) - 2L))) if (!r_version %in% version_names) { # This can only trip if e.g. oldrel-99 is requested - stop("`r_version` must be a version number or one of ", toString(sQuote(version_names)), call. = FALSE) + cli::cli_abort("{.var r_version} must be a version number or one of {toString(sQuote(version_names))}") } requested_version <- minor_versions[match(r_version, table = version_names)] available_patches <- all_versions[startsWith(all_versions, requested_version)] @@ -102,13 +102,10 @@ normalize_r_version <- function(r_version) { } else if (is.character(r_version)) { r_version <- R_system_version(r_version, strict = TRUE) } else if (!inherits(r_version, "R_system_version")) { - stop("`r_version` must be a R version number, returned by R_system_version(), or a string.", call. = FALSE) + cli::cli_abort("{.var r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") } if (r_version < "3.0.0") { - warning( - "It is not recommended to depend on an R version older than 3.0.0. Resetting 'r_version' to 3.0.0.", - call. = FALSE - ) + cli::cli_warn("Depending on an R version older than 3.0.0 is not recommended. Resetting {.var r_version} to 3.0.0.") r_version <- R_system_version("3.0.0") } r_version From 9f578d2ef26bf83f29fac606718006c3f7a63522 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Tue, 12 Dec 2023 08:05:40 +0100 Subject: [PATCH 02/65] cover addin and settings --- R/addins.R | 12 +++++++----- tests/testthat/_snaps/backport_linter.md | 8 ++++++++ tests/testthat/_snaps/settings.md | 10 ++++++++++ tests/testthat/test-backport_linter.R | 5 ++--- tests/testthat/test-settings.R | 2 +- 5 files changed, 28 insertions(+), 9 deletions(-) create mode 100644 tests/testthat/_snaps/backport_linter.md create mode 100644 tests/testthat/_snaps/settings.md diff --git a/R/addins.R b/R/addins.R index 258ba8d64..6bf0beee9 100644 --- a/R/addins.R +++ b/R/addins.R @@ -1,11 +1,11 @@ # nocov start addin_lint <- function() { if (!requireNamespace("rstudioapi", quietly = TRUE)) { - stop("'rstudioapi' is required for add-ins.", call. = FALSE) + cli::cli_abort("{.pkg rstudioapi} is required for add-ins.") } filename <- rstudioapi::getSourceEditorContext() if (filename$path == "") { - return("Current source has no path. Please save before continue") + return("Current source has no path. Please save before continue.") } config_file <- (get("find_config", asNamespace("lintr")))(filename$path) @@ -16,7 +16,7 @@ addin_lint <- function() { config_linters <- config[["linters"]] } linters <- if (length(config_linters) == 0L) { - message("No configuration found. Using default linters.") + cli::cli_inform("No configuration found. Using default linters.") default_linters } else { eval(parse(text = config_linters)) @@ -27,12 +27,14 @@ addin_lint <- function() { addin_lint_package <- function() { if (!requireNamespace("rstudioapi", quietly = TRUE)) { - stop("'rstudioapi' is required for add-ins.", call. = FALSE) + cli::cli_abort("{.pkg rstudioapi} is required for add-ins.") } project <- rstudioapi::getActiveProject() project_path <- if (is.null(project)) getwd() else project - if (is.null(project)) message("No project found, passing current directory") + if (is.null(project)) { + cli::cli_inform("No project found, passing current directory") + } lintr::lint_package(project_path) } diff --git a/tests/testthat/_snaps/backport_linter.md b/tests/testthat/_snaps/backport_linter.md new file mode 100644 index 000000000..3b29892a5 --- /dev/null +++ b/tests/testthat/_snaps/backport_linter.md @@ -0,0 +1,8 @@ +# backport_linter produces error when R version misspecified + + Code + lint(text = "numToBits(2)", linters = backport_linter(420L)) + Condition + Error in `normalize_r_version()`: + ! `r_version` must be an R version number, returned by `R_system_version()`, or a string. + diff --git a/tests/testthat/_snaps/settings.md b/tests/testthat/_snaps/settings.md new file mode 100644 index 000000000..fc7b37f43 --- /dev/null +++ b/tests/testthat/_snaps/settings.md @@ -0,0 +1,10 @@ +# read_config_file() bubbles up warnings helpfully, without erroring (#2253) + + Code + lint_dir() + Condition + Warning: + Warning encountered while loading config: + Warning from config setting 'linters' in 'NULL': + Depending on an R version older than 3.0.0 is not recommended. Resetting `r_version` to 3.0.0. + diff --git a/tests/testthat/test-backport_linter.R b/tests/testthat/test-backport_linter.R index 0ecb9902a..73427bccf 100644 --- a/tests/testthat/test-backport_linter.R +++ b/tests/testthat/test-backport_linter.R @@ -1,8 +1,7 @@ test_that("backport_linter produces error when R version misspecified", { - expect_error( + expect_snapshot( lint(text = "numToBits(2)", linters = backport_linter(420L)), - "`r_version` must be a R version number, returned by R_system_version(), or a string.", - fixed = TRUE + error = TRUE ) }) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index f9350c6a0..6e3b3c20e 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -271,7 +271,7 @@ test_that("read_config_file() bubbles up warnings helpfully, without erroring (# 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") + expect_snapshot(lint_dir()) }) test_that("perl-only regular expressions are accepted in config", { From d4371548c743adea145f90641e292fb98439ed27 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Tue, 12 Dec 2023 08:40:36 +0100 Subject: [PATCH 03/65] Update DESCRIPTION --- DESCRIPTION | 1 - 1 file changed, 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index 3eb5a0dcf..0a2c20eb3 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -37,7 +37,6 @@ Imports: xmlparsedata (>= 1.0.5) Suggests: bookdown, - cli, httr (>= 1.2.1), jsonlite, patrick (>= 0.2.0), From d69c1af8585a0f26e2132e061df1ad5f450bb055 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 13 Dec 2023 07:09:25 +0100 Subject: [PATCH 04/65] import cli functions --- NAMESPACE | 3 +++ R/addins.R | 8 ++++---- R/backport_linter.R | 6 +++--- R/lintr-package.R | 1 + 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 3b4b473bf..6b597dea2 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -170,6 +170,9 @@ export(with_id) export(xml_nodes_to_lints) export(xp_call_name) export(yoda_test_linter) +importFrom(cli,cli_abort) +importFrom(cli,cli_inform) +importFrom(cli,cli_warn) importFrom(cyclocomp,cyclocomp) importFrom(glue,glue) importFrom(glue,glue_collapse) diff --git a/R/addins.R b/R/addins.R index 6bf0beee9..ed59ba3d6 100644 --- a/R/addins.R +++ b/R/addins.R @@ -1,7 +1,7 @@ # nocov start addin_lint <- function() { if (!requireNamespace("rstudioapi", quietly = TRUE)) { - cli::cli_abort("{.pkg rstudioapi} is required for add-ins.") + cli_abort("{.pkg rstudioapi} is required for add-ins.") } filename <- rstudioapi::getSourceEditorContext() if (filename$path == "") { @@ -16,7 +16,7 @@ addin_lint <- function() { config_linters <- config[["linters"]] } linters <- if (length(config_linters) == 0L) { - cli::cli_inform("No configuration found. Using default linters.") + cli_inform("No configuration found. Using default linters.") default_linters } else { eval(parse(text = config_linters)) @@ -27,13 +27,13 @@ addin_lint <- function() { addin_lint_package <- function() { if (!requireNamespace("rstudioapi", quietly = TRUE)) { - cli::cli_abort("{.pkg rstudioapi} is required for add-ins.") + cli_abort("{.pkg rstudioapi} is required for add-ins.") } project <- rstudioapi::getActiveProject() project_path <- if (is.null(project)) getwd() else project if (is.null(project)) { - cli::cli_inform("No project found, passing current directory") + cli_inform("No project found, passing current directory") } lintr::lint_package(project_path) diff --git a/R/backport_linter.R b/R/backport_linter.R index a8ed81f12..dcaa5f172 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -90,7 +90,7 @@ normalize_r_version <- function(r_version) { version_names <- c("devel", "release", paste0("oldrel-", seq_len(length(minor_versions) - 2L))) if (!r_version %in% version_names) { # This can only trip if e.g. oldrel-99 is requested - cli::cli_abort("{.var r_version} must be a version number or one of {toString(sQuote(version_names))}") + cli_abort("{.var r_version} must be a version number or one of {toString(sQuote(version_names))}") } requested_version <- minor_versions[match(r_version, table = version_names)] available_patches <- all_versions[startsWith(all_versions, requested_version)] @@ -102,10 +102,10 @@ normalize_r_version <- function(r_version) { } else if (is.character(r_version)) { r_version <- R_system_version(r_version, strict = TRUE) } else if (!inherits(r_version, "R_system_version")) { - cli::cli_abort("{.var r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") + cli_abort("{.var r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") } if (r_version < "3.0.0") { - cli::cli_warn("Depending on an R version older than 3.0.0 is not recommended. Resetting {.var r_version} to 3.0.0.") + cli_warn("Depending on an R version older than 3.0.0 is not recommended. Resetting {.var r_version} to 3.0.0.") r_version <- R_system_version("3.0.0") } r_version diff --git a/R/lintr-package.R b/R/lintr-package.R index 898db1445..8f87aea99 100644 --- a/R/lintr-package.R +++ b/R/lintr-package.R @@ -8,6 +8,7 @@ "_PACKAGE" ## lintr namespace: start +#' @importFrom cli cli_inform cli_abort cli_warn #' @importFrom cyclocomp cyclocomp #' @importFrom glue glue glue_collapse #' @importFrom rex rex regex re_matches re_substitutes character_class From b39be6706793682df68db5be44409c9159c20825 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 07:38:59 +0100 Subject: [PATCH 05/65] undo snapshot tests --- tests/testthat/_snaps/backport_linter.md | 8 -------- tests/testthat/_snaps/settings.md | 10 ---------- tests/testthat/test-backport_linter.R | 4 ++-- tests/testthat/test-settings.R | 5 ++++- 4 files changed, 6 insertions(+), 21 deletions(-) delete mode 100644 tests/testthat/_snaps/backport_linter.md delete mode 100644 tests/testthat/_snaps/settings.md diff --git a/tests/testthat/_snaps/backport_linter.md b/tests/testthat/_snaps/backport_linter.md deleted file mode 100644 index 3b29892a5..000000000 --- a/tests/testthat/_snaps/backport_linter.md +++ /dev/null @@ -1,8 +0,0 @@ -# backport_linter produces error when R version misspecified - - Code - lint(text = "numToBits(2)", linters = backport_linter(420L)) - Condition - Error in `normalize_r_version()`: - ! `r_version` must be an R version number, returned by `R_system_version()`, or a string. - diff --git a/tests/testthat/_snaps/settings.md b/tests/testthat/_snaps/settings.md deleted file mode 100644 index fc7b37f43..000000000 --- a/tests/testthat/_snaps/settings.md +++ /dev/null @@ -1,10 +0,0 @@ -# read_config_file() bubbles up warnings helpfully, without erroring (#2253) - - Code - lint_dir() - Condition - Warning: - Warning encountered while loading config: - Warning from config setting 'linters' in 'NULL': - Depending on an R version older than 3.0.0 is not recommended. Resetting `r_version` to 3.0.0. - diff --git a/tests/testthat/test-backport_linter.R b/tests/testthat/test-backport_linter.R index 73427bccf..4da65886d 100644 --- a/tests/testthat/test-backport_linter.R +++ b/tests/testthat/test-backport_linter.R @@ -1,7 +1,7 @@ test_that("backport_linter produces error when R version misspecified", { - expect_snapshot( + expect_error( lint(text = "numToBits(2)", linters = backport_linter(420L)), - error = TRUE + "`r_version` must be an R version number" ) }) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 6e3b3c20e..a09012e22 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -271,7 +271,10 @@ test_that("read_config_file() bubbles up warnings helpfully, without erroring (# withr::local_dir(withr::local_tempdir()) writeLines("a <- 1", "aaa.R") - expect_snapshot(lint_dir()) + expect_warning( + lint_dir(), + "Depending on an R version older than 3.0.0 is not recommended" + ) }) test_that("perl-only regular expressions are accepted in config", { From 2a797f8cae4e0cbd5a46fcd657a921dbd9d621b1 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 07:48:41 +0100 Subject: [PATCH 06/65] address initial review comments --- R/addins.R | 2 +- R/backport_linter.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/addins.R b/R/addins.R index ed59ba3d6..98dbcbad5 100644 --- a/R/addins.R +++ b/R/addins.R @@ -5,7 +5,7 @@ addin_lint <- function() { } filename <- rstudioapi::getSourceEditorContext() if (filename$path == "") { - return("Current source has no path. Please save before continue.") + return("Current source has no path. Please save before continuing.") } config_file <- (get("find_config", asNamespace("lintr")))(filename$path) diff --git a/R/backport_linter.R b/R/backport_linter.R index 40829afbe..4f4fda522 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -93,7 +93,7 @@ normalize_r_version <- function(r_version) { version_names <- c("devel", "release", paste0("oldrel-", seq_len(length(minor_versions) - 2L))) if (!r_version %in% version_names) { # This can only trip if e.g. oldrel-99 is requested - cli_abort("{.var r_version} must be a version number or one of {toString(sQuote(version_names))}") + cli_abort("{sQuote(r_version)} must be a version number or one of {toString(sQuote(version_names))}") } requested_version <- minor_versions[match(r_version, table = version_names)] available_patches <- all_versions[startsWith(all_versions, requested_version)] From f885355c826b1cb55d4549a6a3759dbc4170aa30 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 07:55:01 +0100 Subject: [PATCH 07/65] fix test --- tests/testthat/test-backport_linter.R | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-backport_linter.R b/tests/testthat/test-backport_linter.R index 4da65886d..fd7169afc 100644 --- a/tests/testthat/test-backport_linter.R +++ b/tests/testthat/test-backport_linter.R @@ -43,7 +43,10 @@ test_that("backport_linter detects backwards-incompatibility", { backport_linter("oldrel") ) - expect_error(backport_linter("oldrel-99"), "`r_version` must be a version number or one of") + expect_error( + backport_linter("oldrel-99"), + "'oldrel-99' must be a version number or one of" + ) expect_lint( "numToBits(2)", From 474b866589792f6f22dbeb24da2bf82029ec045b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 08:02:52 +0100 Subject: [PATCH 08/65] improve error message --- R/backport_linter.R | 6 +++++- tests/testthat/test-backport_linter.R | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/R/backport_linter.R b/R/backport_linter.R index 4f4fda522..fcb7f58d1 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -93,7 +93,11 @@ normalize_r_version <- function(r_version) { version_names <- c("devel", "release", paste0("oldrel-", seq_len(length(minor_versions) - 2L))) if (!r_version %in% version_names) { # This can only trip if e.g. oldrel-99 is requested - cli_abort("{sQuote(r_version)} must be a version number or one of {toString(sQuote(version_names))}") + cli_abort(c( + "{.var r_version} is not valid:", + "i" = "It must be a version number or one of {toString(sQuote(version_names))}", + "x" = "You entered {toString(sQuote(r_version))} instead" + )) } requested_version <- minor_versions[match(r_version, table = version_names)] available_patches <- all_versions[startsWith(all_versions, requested_version)] diff --git a/tests/testthat/test-backport_linter.R b/tests/testthat/test-backport_linter.R index fd7169afc..801a2719f 100644 --- a/tests/testthat/test-backport_linter.R +++ b/tests/testthat/test-backport_linter.R @@ -45,7 +45,7 @@ test_that("backport_linter detects backwards-incompatibility", { expect_error( backport_linter("oldrel-99"), - "'oldrel-99' must be a version number or one of" + "`r_version` is not valid" ) expect_lint( From c406b7a258fac303bbf693f2a41a2d4e1a999ba7 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 08:09:48 +0100 Subject: [PATCH 09/65] delint --- R/backport_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/backport_linter.R b/R/backport_linter.R index fcb7f58d1..4545954e4 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -95,8 +95,8 @@ normalize_r_version <- function(r_version) { # This can only trip if e.g. oldrel-99 is requested cli_abort(c( "{.var r_version} is not valid:", - "i" = "It must be a version number or one of {toString(sQuote(version_names))}", - "x" = "You entered {toString(sQuote(r_version))} instead" + i = "It must be a version number or one of {toString(sQuote(version_names))}", + x = "You entered {toString(sQuote(r_version))} instead" )) } requested_version <- minor_versions[match(r_version, table = version_names)] From 6fe595f2f7f122979128718473582a30a88b8ffc Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 14 Dec 2023 22:45:47 +0100 Subject: [PATCH 10/65] use cli in comments --- R/comments.R | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/R/comments.R b/R/comments.R index 5ada6e84f..41d349dc6 100644 --- a/R/comments.R +++ b/R/comments.R @@ -84,10 +84,10 @@ wercker_build_info <- function() { # nocov start github_comment <- function(text, info = NULL, token = settings$comment_token) { if (!requireNamespace("httr", quietly = TRUE)) { - stop("Package 'httr' is required to post comments with github_comment().", call. = FALSE) + cli_abort("Package {.pkg httr} is required to post comments with {.fn github_comment}.") } if (!requireNamespace("jsonlite", quietly = TRUE)) { - stop("Package 'jsonlite' is required to post comments with github_comment().", call. = FALSE) + cli_abort("Package {.pkg jsonlite} is required to post comments with {.fn github_comment}.") } if (is.null(info)) { @@ -99,7 +99,10 @@ github_comment <- function(text, info = NULL, token = settings$comment_token) { } else if (!is.null(info$commit)) { api_subdir <- file.path("commits", info$commit) } else { - stop("Expected a pull or a commit, but received ci_build_info() = ", format(info), call. = FALSE) + cli_abort(c( + "Expected a pull or a commit", + "x" = "Instead received {.fn ci_build_info} = {format(info)}" + )) } response <- httr::POST( "https://api.github.com", From 793277ff8fcdd52c50a8ea5b7d4c83513e68cce8 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 15 Dec 2023 17:49:41 +0100 Subject: [PATCH 11/65] Update linter_tags.R --- R/linter_tags.R | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/R/linter_tags.R b/R/linter_tags.R index 3bf581013..fb8588f51 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -49,13 +49,13 @@ #' @export available_linters <- function(packages = "lintr", tags = NULL, exclude_tags = "deprecated") { if (!is.character(packages)) { - stop("`packages` must be a character vector.", call. = FALSE) + cli_abort("{.arg packages} must be a character vector.") } if (!is.null(tags) && !is.character(tags)) { - stop("`tags` must be a character vector.", call. = FALSE) + cli_abort("{.arg tags} must be a character vector.") } if (!is.null(exclude_tags) && !is.character(exclude_tags)) { - stop("`exclude_tags` must be a character vector.", call. = FALSE) + cli_abort("{.arg exclude_tags} must be a character vector.") } # any tags specified explicitly will not be excluded (#1959) @@ -117,13 +117,10 @@ validate_linter_db <- function(available, package) { # Check that the csv file contains two character columns, named 'linter' and 'tags'. # Otherwise, fallback to an empty data frame. if (!all(c("linter", "tags") %in% colnames(available))) { - warning( - "`linters.csv` must contain the columns 'linter' and 'tags'.\nPackage '", - package, "' is missing ", - paste0("'", setdiff(c("linter", "tags"), names(available)), "'", collapse = " and "), - ".", - call. = FALSE - ) + cli_warn(c( + "i" = "{.file linters.csv} must contain the columns {.str linter} and {.str tags}.", + "x" = "Package {.pkg package} is missing {.str {setdiff(c('linter', 'tags'), names(available))}}." + )) return(FALSE) } nrow(available) > 0L @@ -153,7 +150,7 @@ rd_tags <- function(linter_name) { linters <- available_linters(exclude_tags = NULL) tags <- platform_independent_sort(linters[["tags"]][[match(linter_name, linters[["linter"]])]]) if (length(tags) == 0L) { - stop("tags are required, but found none for ", linter_name, call. = FALSE) + cli_abort("Tags are required, but found none for {.fn linter_name}.") } c( @@ -172,7 +169,7 @@ rd_linters <- function(tag_name) { linters <- available_linters(tags = tag_name) tagged <- platform_independent_sort(linters[["linter"]]) if (length(tagged) == 0L) { - stop("No linters found associated with tag ", tag_name, call. = FALSE) + cli_abort("No linters found associated with tag {.emph tag_name}.") } c( From 36f55d2821c6c94fe54e91c8345194f46b58faab Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 15 Dec 2023 18:19:59 +0100 Subject: [PATCH 12/65] fix test and delint --- R/comments.R | 2 +- R/linter_tags.R | 4 ++-- tests/testthat/test-linter_tags.R | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/comments.R b/R/comments.R index 41d349dc6..8d1040e17 100644 --- a/R/comments.R +++ b/R/comments.R @@ -101,7 +101,7 @@ github_comment <- function(text, info = NULL, token = settings$comment_token) { } else { cli_abort(c( "Expected a pull or a commit", - "x" = "Instead received {.fn ci_build_info} = {format(info)}" + x = "Instead received {.fn ci_build_info} = {format(info)}" )) } response <- httr::POST( diff --git a/R/linter_tags.R b/R/linter_tags.R index fb8588f51..69db2b8e9 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -118,8 +118,8 @@ validate_linter_db <- function(available, package) { # Otherwise, fallback to an empty data frame. if (!all(c("linter", "tags") %in% colnames(available))) { cli_warn(c( - "i" = "{.file linters.csv} must contain the columns {.str linter} and {.str tags}.", - "x" = "Package {.pkg package} is missing {.str {setdiff(c('linter', 'tags'), names(available))}}." + i = "{.file linters.csv} must contain the columns {.str linter} and {.str tags}.", + x = "Package {.pkg package} is missing {.str {setdiff(c('linter', 'tags'), names(available))}}." )) return(FALSE) } diff --git a/tests/testthat/test-linter_tags.R b/tests/testthat/test-linter_tags.R index 218c813ee..d3b1a6fc3 100644 --- a/tests/testthat/test-linter_tags.R +++ b/tests/testthat/test-linter_tags.R @@ -8,7 +8,7 @@ test_that("validate_linter_db works as expected", { df_empty <- data.frame() expect_warning( lintr:::validate_linter_db(df_empty, "mypkg"), - "`linters.csv` must contain the columns 'linter' and 'tags'.", + 'must contain the columns "linter" and "tags"', fixed = TRUE ) expect_false(suppressWarnings(lintr:::validate_linter_db(df_empty, "mypkg"))) From 72bb926de044778d40130c7b94ce519b28f4147c Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 28 Dec 2023 08:18:32 +0100 Subject: [PATCH 13/65] more conversions --- R/path_utils.R | 4 ++-- R/semicolon_linter.R | 8 ++++---- R/shared_constants.R | 14 ++++++++------ tests/testthat/test-semicolon_linter.R | 2 +- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/R/path_utils.R b/R/path_utils.R index d9c47a99c..a9a8f3a90 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -108,10 +108,10 @@ is_valid_long_path <- function(path, lax = FALSE) { split_paths <- function(path, sep = "/|\\\\") { if (!is.character(path)) { - stop("argument 'path' should be a character vector", call. = FALSE) + cli_abort("Argument {.arg path} should be a character vector.") } if (!is.character(sep) || length(sep) != 1L || !nzchar(sep)) { - stop("argument 'sep' should be a non-empty regular expression character string", call. = FALSE) + cli_abort("Argument {.arg sep} should be a non-empty regular expression character string.") } Map(split_path, strsplit(path, sep), substr(path, 1L, 1L)) } diff --git a/R/semicolon_linter.R b/R/semicolon_linter.R index 5faf5e12c..febef1d8e 100644 --- a/R/semicolon_linter.R +++ b/R/semicolon_linter.R @@ -64,10 +64,10 @@ semicolon_linter <- function(allow_compound = FALSE, allow_trailing = FALSE) { msg_compound <- "Replace compound semicolons by a newline." if (allow_compound && allow_trailing) { - stop( - "At least one of `allow_compound` or `allow_trailing` must be FALSE, otherwise no lints can be generated.", - call. = FALSE - ) + cli_abort(c( + x = "At least one of {.arg allow_compound} or {.arg allow_trailing} must be FALSE.", + i = "No lints can be generated otherwise." + )) } else if (allow_compound && !allow_trailing) { # lint only trailing xpath <- "//OP-SEMICOLON[not(@line1 = following-sibling::*[1]/@line1)]" diff --git a/R/shared_constants.R b/R/shared_constants.R index ef4eccb50..51a24328e 100644 --- a/R/shared_constants.R +++ b/R/shared_constants.R @@ -268,14 +268,16 @@ extract_glued_symbols <- function(expr, interpret_glue) { } unexpected_glue_parse_error <- function(cond) { - stop("Unexpected failure to parse glue call, please report: ", conditionMessage(cond), call. = FALSE) # nocov + cli_abort(c( + x = "Unexpected failure to parse glue call.", + i = "Please report: {conditionMessage(cond)}" + )) # nocov } glue_parse_failure_warning <- function(cond) { - warning( - "Evaluating glue expression while testing for local variable usage failed: ", conditionMessage(cond), - "\nPlease ensure correct glue syntax, e.g., matched delimiters.", - call. = FALSE - ) + cli_warn(c( + x = "Evaluating glue expression while testing for local variable usage failed: {conditionMessage(cond)}", + i = "Please ensure correct glue syntax, e.g., matched delimiters." + )) NULL } glue_symbol_extractor <- function(text, envir, data) { diff --git a/tests/testthat/test-semicolon_linter.R b/tests/testthat/test-semicolon_linter.R index 6cb5dd538..674634699 100644 --- a/tests/testthat/test-semicolon_linter.R +++ b/tests/testthat/test-semicolon_linter.R @@ -99,7 +99,7 @@ test_that("Trailing semicolons only", { test_that("Compound semicolons only", { expect_error( lint(text = "a <- 1;", linters = semicolon_linter(allow_trailing = TRUE, allow_compound = TRUE)), - "At least one of `allow_compound` or `allow_trailing` must be FALSE, otherwise no lints can be generated.", + "At least one of `allow_compound` or `allow_trailing` must be FALSE", fixed = TRUE ) }) From 176fd17b67562abdd083a643ee7f2808f114f966 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 28 Dec 2023 08:35:30 +0100 Subject: [PATCH 14/65] conversions in undesirables --- R/tree_utils.R | 8 ++++---- R/undesirable_function_linter.R | 8 ++++---- R/undesirable_operator_linter.R | 10 +++++----- tests/testthat/test-undesirable_function_linter.R | 2 +- tests/testthat/test-undesirable_operator_linter.R | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/R/tree_utils.R b/R/tree_utils.R index 58024b48d..d895124e0 100644 --- a/R/tree_utils.R +++ b/R/tree_utils.R @@ -10,10 +10,10 @@ generate_top_level_map <- function(pc) { tl_parent[i_not_assigned] <- pc$parent[match(tl_parent[i_not_assigned], pc$id)] i_not_assigned <- which(!tl_parent %in% tl_ids) if (length(i_not_assigned) >= prev_length) { # nocov start - stop( - "Logical error: unassigned set did not shrink. Check file syntax and please report as a lintr bug.", - call. = FALSE - ) + cli_abort(c( + x = "Logical error: unassigned set did not shrink.", + i = "Check file syntax and please report as a {.pkg lintr} bug." + )) } # nocov end } tl_parent diff --git a/R/undesirable_function_linter.R b/R/undesirable_function_linter.R index 762ecda5d..a567e9c05 100644 --- a/R/undesirable_function_linter.R +++ b/R/undesirable_function_linter.R @@ -59,10 +59,10 @@ undesirable_function_linter <- function(fun = default_undesirable_functions, symbol_is_undesirable = TRUE) { stopifnot(is.logical(symbol_is_undesirable)) if (is.null(names(fun)) || !all(nzchar(names(fun))) || length(fun) == 0L) { - stop( - "'fun' should be a non-empty named character vector; use missing elements to indicate default messages.", - call. = FALSE - ) + cli_abort(c( + x = "{.arg fun} should be a non-empty named character vector.", + i = "Use missing elements to indicate default messages." + )) } xp_condition <- xp_and( diff --git a/R/undesirable_operator_linter.R b/R/undesirable_operator_linter.R index 734e6c485..879625cdd 100644 --- a/R/undesirable_operator_linter.R +++ b/R/undesirable_operator_linter.R @@ -44,10 +44,10 @@ #' @export undesirable_operator_linter <- function(op = default_undesirable_operators) { if (is.null(names(op)) || !all(nzchar(names(op))) || length(op) == 0L) { - stop( - "'op' should be a non-empty named character vector; use missing elements to indicate default messages.", - call. = FALSE - ) + cli_abort(c( + x = "{.arg op} should be a non-empty named character vector.", + i = "Use missing elements to indicate default messages." + )) } # infix must be handled individually below; non-assignment `=` are always OK operator_nodes <- infix_metadata$xml_tag_exact[ @@ -61,7 +61,7 @@ undesirable_operator_linter <- function(op = default_undesirable_operators) { } if (length(operator_nodes) == 0L) { - stop("Did not recognize any valid operators in request for: ", toString(names(op)), call. = FALSE) + cli_abort("Did not recognize any valid operators in request for: {toString(names(op))}") } xpath <- paste(paste0("//", operator_nodes), collapse = " | ") diff --git a/tests/testthat/test-undesirable_function_linter.R b/tests/testthat/test-undesirable_function_linter.R index c12cbf21b..ae8f1af1b 100644 --- a/tests/testthat/test-undesirable_function_linter.R +++ b/tests/testthat/test-undesirable_function_linter.R @@ -55,7 +55,7 @@ test_that("Line numbers are extracted correctly", { }) test_that("invalid inputs fail correctly", { - error_msg <- "'fun' should be a non-empty named character vector" + error_msg <- "`fun` should be a non-empty named character vector" expect_error( undesirable_function_linter("***"), diff --git a/tests/testthat/test-undesirable_operator_linter.R b/tests/testthat/test-undesirable_operator_linter.R index 35aa6c20b..a3e2aea2b 100644 --- a/tests/testthat/test-undesirable_operator_linter.R +++ b/tests/testthat/test-undesirable_operator_linter.R @@ -51,7 +51,7 @@ test_that("undesirable_operator_linter vectorizes messages", { }) test_that("invalid inputs fail correctly", { - error_msg <- "'op' should be a non-empty named character vector" + error_msg <- "`op` should be a non-empty named character vector" expect_error( undesirable_operator_linter("***"), From 15bf9a1f54832763422b2d482756747d1ebd3949 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Thu, 28 Dec 2023 10:20:56 +0100 Subject: [PATCH 15/65] Update xml_nodes_to_lints.R --- R/xml_nodes_to_lints.R | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index 323a0f5be..b44a08ed8 100644 --- a/R/xml_nodes_to_lints.R +++ b/R/xml_nodes_to_lints.R @@ -53,17 +53,16 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, class(lints) <- "lints" return(lints) } else if (!is_node(xml)) { - stop( - "Expected an xml_nodeset, a list of xml_nodes, or an xml_node, instead got an object of class(es): ", - toString(class(xml)), - call. = FALSE - ) + cli_abort(c( + x = "Expected an xml_nodeset, a list of xml_nodes, or an xml_node.", + i = "Instead got an object of class(es): {toString(class(xml))}" + )) } type <- match.arg(type, c("style", "warning", "error")) line1 <- xml_attr(xml, "line1") col1 <- xp_find_location(xml, range_start_xpath) if (is.na(col1)) { - warning("Could not find range start for lint. Defaulting to start of line.", call. = FALSE) + cli_warn(c(i = "Could not find range start for lint. Defaulting to start of line.")) col1 <- 1L } @@ -73,7 +72,7 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, if (xml_attr(xml, "line2") == line1) { col2 <- xp_find_location(xml, range_end_xpath) if (is.na(col2)) { - warning("Could not find range end for lint. Defaulting to width 1.", call. = FALSE) + cli_warn(c(i = "Could not find range end for lint. Defaulting to width 1.")) col2 <- col1 } } else { @@ -82,7 +81,7 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, column_number <- xp_find_location(xml, column_number_xpath) if (is.na(column_number)) { - warning("Could not find location for lint. Defaulting to start of range.", call. = FALSE) + cli_warn(c(i = "Could not find location for lint. Defaulting to start of range.")) column_number <- col1 } From dc11fcdbd21f0c45e1b7827eb0f2cea15215ef5e Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 30 Dec 2023 15:37:30 +0100 Subject: [PATCH 16/65] start with settings --- R/backport_linter.R | 5 ++++- R/settings.R | 16 +++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/R/backport_linter.R b/R/backport_linter.R index 4545954e4..110db4b04 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -112,7 +112,10 @@ normalize_r_version <- function(r_version) { cli_abort("{.var r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") } if (r_version < "3.0.0") { - cli_warn("Depending on an R version older than 3.0.0 is not recommended. Resetting {.var r_version} to 3.0.0.") + cli_warn(c( + x = "Depending on an R version older than 3.0.0 is not recommended.", + i = "Resetting {.var r_version} to 3.0.0." + )) r_version <- R_system_version("3.0.0") } r_version diff --git a/R/settings.R b/R/settings.R index 167fcd6ac..47c623342 100644 --- a/R/settings.R +++ b/R/settings.R @@ -153,10 +153,8 @@ read_config_file <- function(config_file) { 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]), - call. = FALSE - ) + unused_settings <- names(config)[!matched] + cli_warn("Found unused settings in config {.str config_file}: {toString(unused_settings)}") } validate_regex(config, @@ -205,11 +203,11 @@ validate_linters <- function(linters) { 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)), ".", - call. = FALSE - ) + non_linters <- which(!is_linters) + cli_abort(c( + i = "Setting 'linters' should be a list of linters.", + x = "Found non-linters at elements: {toString(non_linters)}." + )) } } From 24248f0f68cdd01cd997725c78f5ad0f408b8f3d Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 23 Mar 2024 08:46:12 +0100 Subject: [PATCH 17/65] fix bad merge --- DESCRIPTION | 4 ---- 1 file changed, 4 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 39bd42daf..f2138e0ee 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -37,11 +37,7 @@ Imports: xmlparsedata (>= 1.0.5) Suggests: bookdown, -<<<<<<< HEAD httr (>= 1.2.1), -======= - cli, ->>>>>>> main jsonlite, patrick (>= 0.2.0), rlang, From b65b498e8702d0d151ee27a352663be22c6acb4b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 23 Mar 2024 08:59:53 +0100 Subject: [PATCH 18/65] convert two more warnings to use cli --- DESCRIPTION | 1 - R/cache.R | 9 ++++----- R/with.R | 10 +++++----- tests/testthat/test-with.R | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index f2138e0ee..220de80ee 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -37,7 +37,6 @@ Imports: xmlparsedata (>= 1.0.5) Suggests: bookdown, - httr (>= 1.2.1), jsonlite, patrick (>= 0.2.0), rlang, diff --git a/R/cache.R b/R/cache.R index 1db84212e..2bc3ae4d0 100644 --- a/R/cache.R +++ b/R/cache.R @@ -53,11 +53,10 @@ load_cache <- function(file, path = NULL) { invokeRestart("muffleWarning") }, error = function(e) { - warning( - "Could not load cache file '", file, "':\n", - conditionMessage(e), - call. = FALSE - ) + cli_warn(c( + x = "Could not load cache file {.file {file}}:\n", + i = conditionMessage(e) + )) } ) } # else nothing to do for source file that has no cache diff --git a/R/with.R b/R/with.R index 8a036c1b4..c132d4c46 100644 --- a/R/with.R +++ b/R/with.R @@ -181,11 +181,11 @@ all_linters <- function(..., packages = "lintr") { linters_with_defaults <- function(..., defaults = default_linters) { dots <- list(...) if (missing(defaults) && "default" %in% names(dots)) { - warning( - "'default' is not an argument to linters_with_defaults(). Did you mean 'defaults'? ", - "This warning will be removed when with_defaults() is fully deprecated.", - call. = FALSE - ) + cli_warn(c( + x = "{.arg default} is not an argument to {.fun linters_with_defaults}.", + i = "Did you mean {.arg defaults}?", + i = "This warning will be removed when {.fun with_defaults} is fully deprecated." + )) defaults <- dots$default nms <- names2(dots) missing_index <- !nzchar(nms, keepNA = TRUE) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 0e9a7f7e6..02f10cf28 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -94,7 +94,7 @@ test_that("linters_with_defaults(default = .) is supported with a deprecation wa { linters <- linters_with_defaults(default = list(), whitespace_linter()) }, - "'default'" + "`default` is not an argument" ) expect_named(linters, "whitespace_linter") From 0fda18e9658e76769ee2852fa1e231ea5dc2fb3a Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 23 Mar 2024 10:21:04 +0100 Subject: [PATCH 19/65] fix lints and cover more warnings with cli --- R/backport_linter.R | 4 +-- R/exclude.R | 10 +++---- R/extract.R | 2 +- R/lint.R | 7 ++--- R/settings.R | 4 +-- R/with.R | 10 +++---- R/xml_nodes_to_lints.R | 15 ++++++++-- tests/testthat/_snaps/xml_nodes_to_lints.md | 33 +++++++++++++++++++++ tests/testthat/test-backport_linter.R | 2 +- tests/testthat/test-settings.R | 2 +- tests/testthat/test-xml_nodes_to_lints.R | 15 ++++------ 11 files changed, 69 insertions(+), 35 deletions(-) create mode 100644 tests/testthat/_snaps/xml_nodes_to_lints.md diff --git a/R/backport_linter.R b/R/backport_linter.R index 110db4b04..257e76191 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -113,8 +113,8 @@ normalize_r_version <- function(r_version) { } if (r_version < "3.0.0") { cli_warn(c( - x = "Depending on an R version older than 3.0.0 is not recommended.", - i = "Resetting {.var r_version} to 3.0.0." + x = "Depending on an R version older than {.val 3.0.0} is not recommended.", + i = "Resetting {.var r_version} to {.val 3.0.0}." )) r_version <- R_system_version("3.0.0") } diff --git a/R/exclude.R b/R/exclude.R index a5462c290..26c2c23b5 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -201,12 +201,10 @@ add_exclusions <- function(exclusions, lines, linters_string, exclude_linter_sep matched <- !is.na(idxs) if (!all(matched)) { bad <- excluded_linters[!matched] - warning( - "Could not find linter", if (length(bad) > 1L) "s" else "", " named ", - glue_collapse(sQuote(bad), sep = ", ", last = " and "), - " in the list of active linters. Make sure the linter is uniquely identified by the given name or prefix.", - call. = FALSE - ) + cli_warn(c( + x = "Could not find linter{?s} named {.field {sQuote(bad)}} in the list of active linters.", + i = "Make sure the linter is uniquely identified by the given name or prefix." + )) } excluded_linters[matched] <- linter_names[idxs[matched]] } diff --git a/R/extract.R b/R/extract.R index 4c21e41df..3f35e09a4 100644 --- a/R/extract.R +++ b/R/extract.R @@ -51,7 +51,7 @@ get_knitr_pattern <- function(filename, lines) { ("knitr" %:::% "detect_pattern")(lines, tolower(("knitr" %:::% "file_ext")(filename))), warning = function(cond) { if (!grepl("invalid UTF-8", conditionMessage(cond), fixed = TRUE)) { - warning(cond, call. = FALSE) + cli_warn(cond) } invokeRestart("muffleWarning") } diff --git a/R/lint.R b/R/lint.R index 4435cb081..c9bee7a06 100644 --- a/R/lint.R +++ b/R/lint.R @@ -242,10 +242,9 @@ lint_package <- function(path = ".", ..., pkg_path <- find_package(path) if (is.null(pkg_path)) { - warning( - sprintf("Didn't find any R package searching upwards from '%s'.", normalizePath(path)), - call. = FALSE - ) + cli_warn(c( + i = "Didn't find any R package searching upwards from {.file {normalizePath(path)}}" + )) return(NULL) } diff --git a/R/settings.R b/R/settings.R index 47c623342..2f5613ab8 100644 --- a/R/settings.R +++ b/R/settings.R @@ -153,7 +153,7 @@ read_config_file <- function(config_file) { validate_config_file <- function(config, config_file, defaults) { matched <- names(config) %in% names(defaults) if (!all(matched)) { - unused_settings <- names(config)[!matched] + unused_settings <- names(config)[!matched] # nolint: object_usage_linter. cli_warn("Found unused settings in config {.str config_file}: {toString(unused_settings)}") } @@ -203,7 +203,7 @@ validate_linters <- function(linters) { is_linters <- vapply(linters, is_linter, logical(1L)) if (!all(is_linters)) { - non_linters <- which(!is_linters) + non_linters <- which(!is_linters) # nolint: object_usage_linter. cli_abort(c( i = "Setting 'linters' should be a list of linters.", x = "Found non-linters at elements: {toString(non_linters)}." diff --git a/R/with.R b/R/with.R index c132d4c46..605c139df 100644 --- a/R/with.R +++ b/R/with.R @@ -45,11 +45,9 @@ modify_defaults <- function(defaults, ...) { if (!all(nms[to_null] %in% names(defaults))) { bad_nms <- setdiff(nms[to_null], names(defaults)) is_are <- if (length(bad_nms) > 1L) "are" else "is" - warning( - "Trying to remove ", glue_collapse(sQuote(bad_nms), sep = ", ", last = " and "), - ", which ", is_are, " not in `defaults`.", - call. = FALSE - ) + cli_warn(c( + i = "Trying to remove {.field {sQuote(bad_nms)}}, which {is_are} not in {.arg defaults}." + )) } is.na(vals) <- nms == vals @@ -182,8 +180,8 @@ 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 = "Did you mean {.arg defaults}?", i = "This warning will be removed when {.fun with_defaults} is fully deprecated." )) defaults <- dots$default diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index b44a08ed8..bb4126ff8 100644 --- a/R/xml_nodes_to_lints.R +++ b/R/xml_nodes_to_lints.R @@ -62,7 +62,10 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, line1 <- xml_attr(xml, "line1") col1 <- xp_find_location(xml, range_start_xpath) if (is.na(col1)) { - cli_warn(c(i = "Could not find range start for lint. Defaulting to start of line.")) + cli_warn(c( + x = "Could not find range start for lint.", + i = "Defaulting to start of line." + )) col1 <- 1L } @@ -72,7 +75,10 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, if (xml_attr(xml, "line2") == line1) { col2 <- xp_find_location(xml, range_end_xpath) if (is.na(col2)) { - cli_warn(c(i = "Could not find range end for lint. Defaulting to width 1.")) + cli_warn(c( + x = "Could not find range end for lint.", + i = "Defaulting to width 1." + )) col2 <- col1 } } else { @@ -81,7 +87,10 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, column_number <- xp_find_location(xml, column_number_xpath) if (is.na(column_number)) { - cli_warn(c(i = "Could not find location for lint. Defaulting to start of range.")) + cli_warn(c( + x = "Could not find location for lint.", + i = "Defaulting to start of range." + )) column_number <- col1 } diff --git a/tests/testthat/_snaps/xml_nodes_to_lints.md b/tests/testthat/_snaps/xml_nodes_to_lints.md new file mode 100644 index 000000000..cfed5d323 --- /dev/null +++ b/tests/testthat/_snaps/xml_nodes_to_lints.md @@ -0,0 +1,33 @@ +# it doesn't produce invalid lints + + Code + lints1 <- xml_nodes_to_lints(xml = node, source_expression = expr, + lint_message = "lint_msg", column_number_xpath = xp_column_number, + range_start_xpath = xp_invalid, range_end_xpath = xp_range_end) + Condition + Warning: + x Could not find range start for lint. + i Defaulting to start of line. + +--- + + Code + lints2 <- xml_nodes_to_lints(xml = node, source_expression = expr, + lint_message = "lint_msg", column_number_xpath = xp_column_number, + range_start_xpath = xp_range_start, range_end_xpath = xp_invalid) + Condition + Warning: + x Could not find range end for lint. + i Defaulting to width 1. + +--- + + Code + lints3 <- xml_nodes_to_lints(xml = node, source_expression = expr, + lint_message = "lint_msg", column_number_xpath = xp_invalid, + range_start_xpath = xp_range_start, range_end_xpath = xp_range_end) + Condition + Warning: + x Could not find location for lint. + i Defaulting to start of range. + diff --git a/tests/testthat/test-backport_linter.R b/tests/testthat/test-backport_linter.R index 801a2719f..ac193c2db 100644 --- a/tests/testthat/test-backport_linter.R +++ b/tests/testthat/test-backport_linter.R @@ -72,7 +72,7 @@ test_that("backport_linter generates expected warnings", { { l <- lint(tmp, backport_linter("2.0.0")) }, - "version older than 3.0.0", + 'version older than "3.0.0"', fixed = TRUE ) expect_identical(l, lint(tmp, backport_linter("3.0.0"))) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index bb3b71461..03f3c1f9f 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -273,7 +273,7 @@ test_that("read_config_file() bubbles up warnings helpfully, without erroring (# writeLines("a <- 1", "aaa.R") expect_warning( lint_dir(), - "Depending on an R version older than 3.0.0 is not recommended" + 'Depending on an R version older than "3.0.0" is not recommended' ) }) diff --git a/tests/testthat/test-xml_nodes_to_lints.R b/tests/testthat/test-xml_nodes_to_lints.R index c5e4d9fdd..022b11efb 100644 --- a/tests/testthat/test-xml_nodes_to_lints.R +++ b/tests/testthat/test-xml_nodes_to_lints.R @@ -100,7 +100,7 @@ test_that("it doesn't produce invalid lints", { xp_range_end <- "number(./following-sibling::*[1]/@col2)" xp_invalid <- "number(./DOES-NOT-EXIST/@col1)" - expect_warning( + expect_snapshot( { lints1 <- xml_nodes_to_lints( xml = node, @@ -110,14 +110,13 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_invalid, range_end_xpath = xp_range_end ) - }, - rex::rex("Could not find range start for lint. Defaulting to start of line.") + } ) expect_identical(lints1[["column_number"]], nchar("before") + 1L) expect_identical(lints1[["ranges"]], list(c(1L, nchar(code)))) - expect_warning( + expect_snapshot( { lints2 <- xml_nodes_to_lints( xml = node, @@ -127,14 +126,13 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_range_start, range_end_xpath = xp_invalid ) - }, - rex::rex("Could not find range end for lint. Defaulting to width 1.") + } ) expect_identical(lints2[["column_number"]], nchar("before") + 1L) expect_identical(lints2[["ranges"]], list(c(1L, 1L))) - expect_warning( + expect_snapshot( { lints3 <- xml_nodes_to_lints( xml = node, @@ -144,8 +142,7 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_range_start, range_end_xpath = xp_range_end ) - }, - rex::rex("Could not find location for lint. Defaulting to start of range.") + } ) expect_identical(lints3[["column_number"]], 1L) From 77846ca86cc3e6a53f9e5038c0f2c3b58cb448a7 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 23 Mar 2024 10:30:20 +0100 Subject: [PATCH 20/65] remove lints --- R/exclude.R | 2 +- R/with.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/exclude.R b/R/exclude.R index 26c2c23b5..b2277c012 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -200,7 +200,7 @@ add_exclusions <- function(exclusions, lines, linters_string, exclude_linter_sep idxs <- pmatch(excluded_linters, linter_names, duplicates.ok = TRUE) matched <- !is.na(idxs) if (!all(matched)) { - bad <- excluded_linters[!matched] + bad <- excluded_linters[!matched] # nolint: object_usage_linter. cli_warn(c( x = "Could not find linter{?s} named {.field {sQuote(bad)}} in the list of active linters.", i = "Make sure the linter is uniquely identified by the given name or prefix." diff --git a/R/with.R b/R/with.R index 605c139df..0cec32886 100644 --- a/R/with.R +++ b/R/with.R @@ -44,7 +44,7 @@ modify_defaults <- function(defaults, ...) { to_null <- vapply(vals, is.null, logical(1L)) if (!all(nms[to_null] %in% names(defaults))) { bad_nms <- setdiff(nms[to_null], names(defaults)) - is_are <- if (length(bad_nms) > 1L) "are" else "is" + is_are <- if (length(bad_nms) > 1L) "are" else "is" # nolint: object_usage_linter. cli_warn(c( i = "Trying to remove {.field {sQuote(bad_nms)}}, which {is_are} not in {.arg defaults}." )) From 6c6985880393c56c9ce26cffeb5492ff4c8ea835 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 23 Mar 2024 12:06:45 +0100 Subject: [PATCH 21/65] don't test full message with snapshot --- tests/testthat/_snaps/xml_nodes_to_lints.md | 33 --------------------- tests/testthat/test-xml_nodes_to_lints.R | 18 +++++++---- 2 files changed, 12 insertions(+), 39 deletions(-) delete mode 100644 tests/testthat/_snaps/xml_nodes_to_lints.md diff --git a/tests/testthat/_snaps/xml_nodes_to_lints.md b/tests/testthat/_snaps/xml_nodes_to_lints.md deleted file mode 100644 index cfed5d323..000000000 --- a/tests/testthat/_snaps/xml_nodes_to_lints.md +++ /dev/null @@ -1,33 +0,0 @@ -# it doesn't produce invalid lints - - Code - lints1 <- xml_nodes_to_lints(xml = node, source_expression = expr, - lint_message = "lint_msg", column_number_xpath = xp_column_number, - range_start_xpath = xp_invalid, range_end_xpath = xp_range_end) - Condition - Warning: - x Could not find range start for lint. - i Defaulting to start of line. - ---- - - Code - lints2 <- xml_nodes_to_lints(xml = node, source_expression = expr, - lint_message = "lint_msg", column_number_xpath = xp_column_number, - range_start_xpath = xp_range_start, range_end_xpath = xp_invalid) - Condition - Warning: - x Could not find range end for lint. - i Defaulting to width 1. - ---- - - Code - lints3 <- xml_nodes_to_lints(xml = node, source_expression = expr, - lint_message = "lint_msg", column_number_xpath = xp_invalid, - range_start_xpath = xp_range_start, range_end_xpath = xp_range_end) - Condition - Warning: - x Could not find location for lint. - i Defaulting to start of range. - diff --git a/tests/testthat/test-xml_nodes_to_lints.R b/tests/testthat/test-xml_nodes_to_lints.R index 022b11efb..c609893d2 100644 --- a/tests/testthat/test-xml_nodes_to_lints.R +++ b/tests/testthat/test-xml_nodes_to_lints.R @@ -100,7 +100,7 @@ test_that("it doesn't produce invalid lints", { xp_range_end <- "number(./following-sibling::*[1]/@col2)" xp_invalid <- "number(./DOES-NOT-EXIST/@col1)" - expect_snapshot( + expect_warning( { lints1 <- xml_nodes_to_lints( xml = node, @@ -110,13 +110,15 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_invalid, range_end_xpath = xp_range_end ) - } + }, + "Defaulting to start of line", + fixed = TRUE ) expect_identical(lints1[["column_number"]], nchar("before") + 1L) expect_identical(lints1[["ranges"]], list(c(1L, nchar(code)))) - expect_snapshot( + expect_warning( { lints2 <- xml_nodes_to_lints( xml = node, @@ -126,13 +128,15 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_range_start, range_end_xpath = xp_invalid ) - } + }, + "Defaulting to width 1", + fixed = TRUE ) expect_identical(lints2[["column_number"]], nchar("before") + 1L) expect_identical(lints2[["ranges"]], list(c(1L, 1L))) - expect_snapshot( + expect_warning( { lints3 <- xml_nodes_to_lints( xml = node, @@ -142,7 +146,9 @@ test_that("it doesn't produce invalid lints", { range_start_xpath = xp_range_start, range_end_xpath = xp_range_end ) - } + }, + "Defaulting to start of range", + fixed = TRUE ) expect_identical(lints3[["column_number"]], 1L) From 6ffc60e95a9ca00433ecd7ed82c1acdecfa494cf Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 29 Mar 2024 07:24:33 +0100 Subject: [PATCH 22/65] Update settings.R --- R/settings.R | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/R/settings.R b/R/settings.R index 2f5613ab8..3778c497b 100644 --- a/R/settings.R +++ b/R/settings.R @@ -134,7 +134,10 @@ read_config_file <- function(config_file) { } } malformed <- function(e) { - stop("Malformed config file:\n ", conditionMessage(e), call. = FALSE) + cli_abort(c( + x = "Malformed config file:", + i = conditionMessage(e) + )) } } withCallingHandlers( @@ -143,7 +146,10 @@ read_config_file <- function(config_file) { error = malformed ), warning = function(w) { - warning("Warning encountered while loading config:\n ", conditionMessage(w), call. = FALSE) + cli::cli_warn(c( + x = "Warning encountered while loading config:", + i = conditionMessage(w) + )) invokeRestart("muffleWarning") } ) @@ -205,7 +211,7 @@ validate_linters <- function(linters) { if (!all(is_linters)) { non_linters <- which(!is_linters) # nolint: object_usage_linter. cli_abort(c( - i = "Setting 'linters' should be a list of linters.", + i = "Setting {.arg linters} should be a list of linters.", x = "Found non-linters at elements: {toString(non_linters)}." )) } From 7231b53a4a0eef0607cd0dc60f96b6ae9109259c Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 29 Mar 2024 07:59:12 +0100 Subject: [PATCH 23/65] fix tests --- tests/testthat/test-settings.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 03f3c1f9f..5a09f7f9c 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -198,10 +198,10 @@ test_that("validate_config_file() detects improperly-formed settings", { 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) + 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) + 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) From 2c7baa957709b3cf585f2f3a8be203ea7878eba1 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 30 Mar 2024 07:15:12 +0100 Subject: [PATCH 24/65] convert two more settings messages --- R/settings.R | 19 +++++++++---------- tests/testthat/test-settings.R | 26 +++++++++++++++----------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/R/settings.R b/R/settings.R index 3778c497b..ec804d53d 100644 --- a/R/settings.R +++ b/R/settings.R @@ -227,11 +227,11 @@ validate_exclusions <- function(exclusions) { 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]), ".", - call. = FALSE - ) + problematic_entries <- toString(which(!has_names)[!unnamed_is_string]) # nolint: object_usage_linter. + cli_abort(c( + i = "Unnamed entries of setting {.arg exclusions} should be strings naming files or directories.", + x = "Check exclusions: {problematic_entries}." + )) } for (ii in which(has_names)) validate_named_exclusion(exclusions, ii) } @@ -244,11 +244,10 @@ validate_named_exclusion <- function(exclusions, idx) { 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, ".", - call. = FALSE - ) + cli_abort(c( + i = "Named entries of setting {.arg exclusions} should designate line numbers for exclusion.", + x = "Check exclusions: {idx}." + )) } } diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 5a09f7f9c..c3eba667a 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -203,35 +203,39 @@ test_that("validate_config_file() detects improperly-formed settings", { writeLines("linters: list(assignment_linter(), 1)", .lintr) expect_error(lint_dir(), "Setting `linters` should be a list of linters", fixed = TRUE) + name_exclusion_error_msg <- "Unnamed entries of setting `exclusions` should be strings" + writeLines("exclusions: list(1L)", .lintr) - expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + expect_error(lint_dir(), name_exclusion_error_msg, fixed = TRUE) writeLines('exclusions: list("aaa.R", 1L)', .lintr) - expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + expect_error(lint_dir(), name_exclusion_error_msg, fixed = TRUE) writeLines("exclusions: list(letters)", .lintr) - expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + expect_error(lint_dir(), name_exclusion_error_msg, fixed = TRUE) writeLines("exclusions: list(NA_character_)", .lintr) - expect_error(lint_dir(), "Unnamed entries of setting 'exclusions' should be strings", fixed = TRUE) + expect_error(lint_dir(), name_exclusion_error_msg, fixed = TRUE) + + exclusion_error_msg <- "Named entries of setting `exclusions` should designate line numbers" writeLines('exclusions: list(aaa.R = "abc")', .lintr) - expect_error(lint_dir(), "Named entries of setting 'exclusions' should designate line numbers", fixed = TRUE) + expect_error(lint_dir(), exclusion_error_msg, 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) + expect_error(lint_dir(), exclusion_error_msg, 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) + expect_error(lint_dir(), exclusion_error_msg, 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) + expect_error(lint_dir(), exclusion_error_msg, 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) + expect_error(lint_dir(), exclusion_error_msg, 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) + expect_error(lint_dir(), exclusion_error_msg, fixed = TRUE) }) test_that("exclusions can be a character vector", { @@ -291,7 +295,7 @@ test_that("settings can be put in a sub-directory", { dir.create(".settings") .lintr <- ".settings/.lintr.R" - writeLines("linters <- list(line_length_linter(10))", .lintr) + writeLines("linters <- list(line_length_linter(10L))", .lintr) dir.create("R") writeLines("abcdefghijklmnopqrstuvwxyz=1", "R/a.R") From e5a4d85fdd6478a1c90ac35a1bd62b3d07ea6fc2 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 31 Mar 2024 08:19:59 +0200 Subject: [PATCH 25/65] convert exclude messages --- NEWS.md | 1 + R/exclude.R | 28 ++++++++++++++-------------- tests/testthat/test-exclusions.R | 4 ++-- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/NEWS.md b/NEWS.md index 95b276e2a..317efad9f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -28,6 +28,7 @@ ## New and improved features +* All user-facing messages are now prepared using the [`{cli}`](https://github.com/r-lib/cli) package (#2418, @IndrajeetPatil). * More helpful errors for invalid configs (#2253, @MichaelChirico). * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). diff --git a/R/exclude.R b/R/exclude.R index b2277c012..02431796c 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -129,7 +129,10 @@ parse_exclusions <- function(file, if (length(starts) != length(ends)) { starts_msg <- line_info(starts, type = "start") ends_msg <- line_info(ends, type = "end") - stop(file, " has ", starts_msg, " but only ", ends_msg, " for exclusion from linting!", call. = FALSE) + cli_abort(c( + i = "Equal number of line starts and ends expected for exclusion from linting.", + x = "{.file {file}} has {starts_msg}, but only {ends_msg}." + )) } for (i in seq_along(starts)) { @@ -246,7 +249,6 @@ normalize_exclusions <- function(x, normalize_path = TRUE, x <- as.list(x) unnamed <- !nzchar(names2(x)) if (any(unnamed)) { - # must be character vectors of length 1 bad <- vapply( seq_along(x), function(i) { @@ -254,14 +256,13 @@ normalize_exclusions <- function(x, normalize_path = TRUE, }, logical(1L) ) + bad_idx <- which(bad) # nolint: object_usage_linter. if (any(bad)) { - stop( - "Full file exclusions must be character vectors of length 1. items: ", - toString(which(bad)), - " are not!", - call. = FALSE - ) + cli_abort(c( + i = "Full file exclusions must be character vectors of length 1.", + x = "Items at following indexes are not: {.val {bad_idx}}." + )) } # Normalize unnamed entries to list( = list(Inf), ...) names(x)[unnamed] <- x[unnamed] @@ -274,14 +275,13 @@ normalize_exclusions <- function(x, normalize_path = TRUE, # must be integer or numeric vectors are_numeric <- vapply(x, is.numeric, logical(1L)) bad <- full_line_exclusions & !are_numeric + bad_idx <- which(bad) # nolint: object_usage_linter. if (any(bad)) { - stop( - "Full line exclusions must be numeric or integer vectors. items: ", - toString(which(bad)), - " are not!", - call. = FALSE - ) + cli_abort(c( + i = "Full line exclusions must be numeric or integer vectors.", + x = "Items at following indexes are not: {.val {bad_idx}}." + )) } # Normalize list( = c()) to diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index ae405b2d5..d8d810928 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -41,7 +41,7 @@ test_that("it gives the expected error message when there is only one start but expect_error( lintr:::parse_exclusions("dummy_projects/project/one_start_no_end.R"), - "has 1 range start (line 3) but only 0 range ends for exclusion from linting", + "has 1 range start (line 3), but only 0 range ends", fixed = TRUE ) }) @@ -51,7 +51,7 @@ test_that("it gives the expected error message when there is mismatch between mu expect_error( lintr:::parse_exclusions("dummy_projects/project/mismatched_starts_ends.R"), - "has 3 range starts (lines 3, 7, 11) but only 2 range ends (lines 1, 9) for exclusion from linting", + "has 3 range starts (lines 3, 7, 11), but only 2 range ends (lines 1, 9)", fixed = TRUE ) }) From eed9b50b1c1ffcc7383582f16bb61d1c7441e4f6 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 31 Mar 2024 08:32:03 +0200 Subject: [PATCH 26/65] fix new lints --- R/exclude.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/exclude.R b/R/exclude.R index 02431796c..0b9354860 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -127,8 +127,8 @@ parse_exclusions <- function(file, if (length(starts) > 0L) { if (length(starts) != length(ends)) { - starts_msg <- line_info(starts, type = "start") - ends_msg <- line_info(ends, type = "end") + starts_msg <- line_info(starts, type = "start") # nolint: object_usage_linter. + ends_msg <- line_info(ends, type = "end") # nolint: object_usage_linter. cli_abort(c( i = "Equal number of line starts and ends expected for exclusion from linting.", x = "{.file {file}} has {starts_msg}, but only {ends_msg}." From 851030c35f533265c01ca467b0ff8f99b00c7f61 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 31 Mar 2024 11:55:02 +0200 Subject: [PATCH 27/65] two more error messages --- R/expect_lint.R | 12 +++++++----- R/lint.R | 5 ++++- man/expect_lint_free.Rd | 2 +- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index 2cc8e1f9e..dc71cc900 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -41,10 +41,12 @@ #' @export expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!requireNamespace("testthat", quietly = TRUE)) { - stop( # nocov start - "'expect_lint' is designed to work within the 'testthat' testing framework, but 'testthat' is not installed.", - call. = FALSE - ) # nocov end + # nocov start + cli_abort(c( + i = "{.fun expect_lint} is designed to work within the {.pkg testthat} testing framework.", + x = "{.pkg testthat} is not installed." + )) + # nocov end } old_lang <- set_lang(language) on.exit(reset_lang(old_lang)) @@ -127,7 +129,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { #' Test that the package is lint free #' #' This function is a thin wrapper around lint_package that simply tests there are no -#' lints in the package. It can be used to ensure that your tests fail if the package +#' lints in the package. It can be used to ensure that your tests fail if the package #' contains lints. #' #' @param ... arguments passed to [lint_package()] diff --git a/R/lint.R b/R/lint.R index c9bee7a06..8977ba889 100644 --- a/R/lint.R +++ b/R/lint.R @@ -237,7 +237,10 @@ lint_package <- function(path = ".", ..., parse_settings = TRUE, show_progress = NULL) { if (length(path) > 1L) { - stop("Only linting one package at a time is supported.", call. = FALSE) + cli_abort(c( + x = "Only linting one package at a time is supported.", + i = "Instead, {.val {length(path)}} package paths were provided." + )) } pkg_path <- find_package(path) diff --git a/man/expect_lint_free.Rd b/man/expect_lint_free.Rd index 87ee5be8f..e4043d7c4 100644 --- a/man/expect_lint_free.Rd +++ b/man/expect_lint_free.Rd @@ -11,6 +11,6 @@ expect_lint_free(...) } \description{ This function is a thin wrapper around lint_package that simply tests there are no -lints in the package. It can be used to ensure that your tests fail if the package +lints in the package. It can be used to ensure that your tests fail if the package contains lints. } From 98899336d9f6a8f74b0981de7610e7e4a6e43608 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 1 Apr 2024 08:57:20 +0200 Subject: [PATCH 28/65] two more error messages --- R/with.R | 13 +++++++------ R/xp_utils.R | 9 ++++----- tests/testthat/test-with.R | 2 +- tests/testthat/test-xp_utils.R | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/R/with.R b/R/with.R index 0cec32886..aec99dd80 100644 --- a/R/with.R +++ b/R/with.R @@ -32,7 +32,7 @@ #' @export modify_defaults <- function(defaults, ...) { if (missing(defaults) || !is.list(defaults) || !all(nzchar(names2(defaults)))) { - stop("`defaults` must be a named list.", call. = FALSE) + cli_abort("{.arg defaults} must be a named list.") } vals <- list(...) nms <- names2(vals) @@ -91,7 +91,7 @@ modify_defaults <- function(defaults, ...) { #' @export linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "deprecated") { if (!is.character(tags) && !is.null(tags)) { - stop("`tags` must be a character vector, or NULL.", call. = FALSE) + cli_abort("{.arg tags} must be a character vector, or {.code NULL}.") } tagged_linters <- list() @@ -209,10 +209,11 @@ call_linter_factory <- function(linter_factory, linter_name, package) { linter <- tryCatch( linter_factory(), error = function(e) { - stop( - "Could not create linter with ", package, "::", linter_name, "(): ", conditionMessage(e), - call. = FALSE - ) + fun_name <- paste0(package, "::", linter_name) # nolint: object_name_linter + cli_abort(c( + i = "Could not create linter with {.fun fun_name}.", + x = conditionMessage(e) + )) } ) # Otherwise, all linters would be called "linter_factory". diff --git a/R/xp_utils.R b/R/xp_utils.R index b96a39f57..6279daa34 100644 --- a/R/xp_utils.R +++ b/R/xp_utils.R @@ -83,11 +83,10 @@ xp_call_name <- function(expr, depth = 1L, condition = NULL) { ) is_valid_expr <- is_node(expr) || is_nodeset(expr) if (!is_valid_expr) { - stop( - "Expected an xml_nodeset or an xml_node, instead got an object of class(es): ", - toString(class(expr)), - call. = FALSE - ) + cli_abort(c( + i = "Expected an {.cls xml_nodeset} or an {.cls xml_node}.", + x = "Instead, {.arg expr} is object of class: {.cls {class(expr)}}" + )) } if (is.null(condition)) { diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 02f10cf28..f1d6b7389 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -5,7 +5,7 @@ test_that("modify_defaults produces error with missing or incorrect defaults", { }) test_that("linters_with_tags produces error with incorrect tags", { - expect_error(linters_with_tags(1L:4L), "`tags` must be a character 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", { diff --git a/tests/testthat/test-xp_utils.R b/tests/testthat/test-xp_utils.R index d00a417d1..5d78cae81 100644 --- a/tests/testthat/test-xp_utils.R +++ b/tests/testthat/test-xp_utils.R @@ -12,7 +12,7 @@ test_that("xp_call_name works", { }) test_that("xp_call_name input validation works", { - expect_error(xp_call_name(2L), "Expected an xml_nodeset", fixed = TRUE) + expect_error(xp_call_name(2L), "Expected an ", fixed = TRUE) xml <- xml2::read_xml("") expect_error(xp_call_name(xml, depth = -1L), "depth >= 0", fixed = TRUE) From 04c45d4400690c2bbea720ffb992b401ab3d1e52 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 1 Apr 2024 10:09:26 +0200 Subject: [PATCH 29/65] more messages in lint --- R/lint.R | 20 +++++++++++++------- R/with.R | 4 ++-- tests/testthat/test-Lint-builder.R | 9 ++++++--- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/R/lint.R b/R/lint.R index 8977ba889..03f599572 100644 --- a/R/lint.R +++ b/R/lint.R @@ -403,17 +403,20 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec } if (length(line) != 1L || !is.character(line)) { - stop("`line` must be a string.", call. = FALSE) + cli_abort("{.arg line} must be a string.", call. = FALSE) } max_col <- max(nchar(line) + 1L, 1L, na.rm = TRUE) if (!is_number(column_number) || column_number < 0L || column_number > max_col) { - stop(sprintf( - "`column_number` must be an integer between 0 and nchar(line) + 1 (%d). It was %s.", - max_col, column_number - ), call. = FALSE) + 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}}." + )) } if (!is_number(line_number) || line_number < 1L) { - stop(sprintf("`line_number` must be a positive integer. It was %s.", line_number), call. = FALSE) + cli_abort(c( + i = "{.arg line_number} must be a positive integer.", + x = "Instead, it was {.val {line_number}}." + )) } check_ranges(ranges, max_col) @@ -448,7 +451,10 @@ check_ranges <- function(ranges, max_col) { return() } if (!is.list(ranges)) { - stop("`ranges` must be NULL or a list.", call. = FALSE) + cli_abort(c( + i = "{.arg ranges} must be {.code NULL} or a {.cls list}.", + x = "Instead, it was {.cls {class(ranges)}}." + )) } for (range in ranges) { diff --git a/R/with.R b/R/with.R index aec99dd80..6bc641337 100644 --- a/R/with.R +++ b/R/with.R @@ -209,9 +209,9 @@ call_linter_factory <- function(linter_factory, linter_name, package) { linter <- tryCatch( linter_factory(), error = function(e) { - fun_name <- paste0(package, "::", linter_name) # nolint: object_name_linter + pkg_fn <- paste0(package, "::", linter_name) # nolint: object_name_linter cli_abort(c( - i = "Could not create linter with {.fun fun_name}.", + i = "Could not create linter with {.fun pkg_fn}.", x = conditionMessage(e) )) } diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index 3d081a1af..4ec7820d7 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -2,15 +2,18 @@ test_that("Lint() errors on invalid input", { dummy_line <- "abc" expect_error( Lint("dummy.R", line = dummy_line, column_number = NA_integer_), - rex::rex("`column_number` must be an integer between 0 and nchar(line) + 1 (4). It was NA.") + "`column_number` must be an integer between 0 and `nchar(line) + 1` (4)", + fixed = TRUE ) expect_error( Lint("dummy.R", line = dummy_line, line_number = 0L), - rex::rex("`line_number` must be a positive integer. It was 0.") + "`line_number` must be a positive integer", + fixed = TRUE ) expect_error( Lint("dummy.R", ranges = c(1L, 3L)), - rex::rex("`ranges` must be NULL or a list.") + "`ranges` must be `NULL` or a ", + fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(1L)), From eb765be3a64d1fd8ba8cce083bf73812774d70d5 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 3 Apr 2024 08:21:53 +0200 Subject: [PATCH 30/65] pick a few low-hanging fruits --- R/expect_lint.R | 9 ++++----- R/lint.R | 15 ++++++++------- R/namespace_linter.R | 9 ++++----- R/object_name_linter.R | 4 ++-- R/object_overwrite_linter.R | 2 +- R/utils.R | 2 +- tests/testthat/test-Lint-builder.R | 9 ++++++--- 7 files changed, 26 insertions(+), 24 deletions(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index dc71cc900..ec4ca9abb 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -108,11 +108,10 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { isTRUE(all.equal(value, check)) } if (!is.logical(ok)) { - stop( - "Invalid regex result, did you mistakenly have a capture group in the regex? ", - "Be sure to escape parenthesis with `[]`", - call. = FALSE - ) + cli_abort(c( + x = "Invalid regex result. Did you mistakenly have a capture group in the regex?", + i = "Be sure to escape parenthesis with `[]`" + )) } testthat::expect(ok, msg) }) diff --git a/R/lint.R b/R/lint.R index 03f599572..e588ad161 100644 --- a/R/lint.R +++ b/R/lint.R @@ -459,18 +459,19 @@ check_ranges <- function(ranges, max_col) { for (range in ranges) { if (!is_number(range, 2L)) { - stop("`ranges` must only contain length 2 integer vectors without NAs.", call. = FALSE) + cli_abort("{.arg ranges} must only contain length 2 integer vectors without {.code NA}s.") } else if (!is_valid_range(range, max_col)) { - stop(sprintf( - "All entries in `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (%d).", max_col - ), call. = FALSE) + cli_abort(c( + x = "Invalid range specified.", + i = "All entries in {.arg ranges} must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 ({.val {max_col}})." + )) } } } rstudio_source_markers <- function(lints) { if (!requireNamespace("rstudioapi", quietly = TRUE)) { - stop("'rstudioapi' is required for rstudio_source_markers().", call. = FALSE) # nocov + cli_abort("{.pkg rstudioapi} is required for {.fn rstudio_source_markers}.") # nocov } # package path will be NULL unless it is a relative path @@ -563,14 +564,14 @@ checkstyle_output <- function(lints, filename = "lintr_results.xml") { #' @export sarif_output <- function(lints, filename = "lintr_results.sarif") { if (!requireNamespace("jsonlite", quietly = TRUE)) { - stop("'jsonlite' is required to produce SARIF reports, please install to continue.", call. = FALSE) # nocov + cli_abort("{.pkg jsonlite} is required to produce SARIF reports. Please install to continue.") # nocov } # package path will be `NULL` unless it is a relative path package_path <- attr(lints, "path") if (is.null(package_path)) { - stop("Package path needs to be a relative path.", call. = FALSE) + cli_abort("Package path needs to be a relative path.") } # setup template diff --git a/R/namespace_linter.R b/R/namespace_linter.R index d6579a86a..4f726420a 100644 --- a/R/namespace_linter.R +++ b/R/namespace_linter.R @@ -84,11 +84,10 @@ namespace_linter <- function(check_exports = TRUE, check_nonexports = TRUE) { # nocov start if (any(failed_namespace)) { - stop( - "Failed to retrieve namespaces for one or more of the packages used with `::` or `:::`. ", - "Please report the issue at https://github.com/r-lib/lintr/issues.", - call. = FALSE - ) + cli_abort(c( + x = "Failed to retrieve namespaces for one or more of the packages used with `::` or `:::`. ", + i = "Please report the issue at {.url https://github.com/r-lib/lintr/issues}." + )) } # nocov end diff --git a/R/object_name_linter.R b/R/object_name_linter.R index bb506692d..54cbb523d 100644 --- a/R/object_name_linter.R +++ b/R/object_name_linter.R @@ -92,7 +92,7 @@ object_name_linter <- function(styles = c("snake_case", "symbols"), regexes = ch } if (length(regexes) > 0L) { if (!is.character(regexes)) { - stop("`regexes` must be a character vector.", call. = FALSE) + cli_abort("{.arg regexes} must be a {.cls character} vector.") } rx_names <- names2(regexes) missing_name <- !nzchar(rx_names) @@ -102,7 +102,7 @@ object_name_linter <- function(styles = c("snake_case", "symbols"), regexes = ch style_list <- c(style_list, as.list(regexes)) } if (length(style_list) == 0L) { - stop("At least one style must be specified using `styles` or `regexes`.", call. = FALSE) + cli_abort("At least one style must be specified using {.arg styles} or {.arg regexes}.") } lint_message <- paste0( diff --git a/R/object_overwrite_linter.R b/R/object_overwrite_linter.R index 6c2eaa27d..3c068503d 100644 --- a/R/object_overwrite_linter.R +++ b/R/object_overwrite_linter.R @@ -56,7 +56,7 @@ object_overwrite_linter <- function( allow_names = character()) { for (package in packages) { if (!requireNamespace(package, quietly = TRUE)) { - stop("Package '", package, "' is not available.", call. = FALSE) + cli_abort("Package {.pkg {package}} is required, but not available.") } } pkg_exports <- lapply( diff --git a/R/utils.R b/R/utils.R index 8d2f92378..e7d2e9eb4 100644 --- a/R/utils.R +++ b/R/utils.R @@ -167,7 +167,7 @@ reset_lang <- function(old_lang) { #' @export Linter <- function(fun, name = linter_auto_name(), linter_level = c(NA_character_, "file", "expression")) { # nolint: object_name, line_length. if (!is.function(fun) || length(formals(args(fun))) != 1L) { - stop("`fun` must be a function taking exactly one argument.", call. = FALSE) + cli_abort("{.arg fun} must be a function taking exactly one argument.") } linter_level <- match.arg(linter_level) force(name) diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index 4ec7820d7..61d05f9fe 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -17,14 +17,17 @@ test_that("Lint() errors on invalid input", { ) expect_error( Lint("dummy.R", ranges = list(1L)), - rex::rex("`ranges` must only contain length 2 integer vectors without NAs.") + "`ranges` must only contain length 2 integer vectors without `NA`s.", + fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(c(1L, NA_integer_))), - rex::rex("`ranges` must only contain length 2 integer vectors without NAs.") + "`ranges` must only contain length 2 integer vectors without `NA`s.", + fixed = TRUE ) expect_error( Lint("dummy.R", line = dummy_line, ranges = list(c(1L, 2L), c(1L, 5L))), - rex::rex("All entries in `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).") + "All entries in `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).", + fixed = TRUE ) }) From e2b8570bfd16e57d6783f3aaad768a084796fc5e Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Fri, 5 Apr 2024 08:08:43 +0200 Subject: [PATCH 31/65] more settings --- R/settings.R | 28 ++++++++++++++++------------ tests/testthat/test-settings.R | 2 +- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/R/settings.R b/R/settings.R index ec804d53d..f0eba02e9 100644 --- a/R/settings.R +++ b/R/settings.R @@ -98,7 +98,10 @@ read_config_file <- function(config_file) { if (endsWith(config_file, ".R")) { load_config <- function(file) sys.source(file, config, keep.source = FALSE, keep.parse.data = FALSE) malformed <- function(e) { - stop("Malformed config file, ensure it is valid R syntax\n ", conditionMessage(e), call. = FALSE) + cli_abort(c( + i = "Malformed config file, ensure it is valid R syntax.", + x = conditionMessage(e) + )) } } else { load_config <- function(file) { @@ -107,26 +110,27 @@ read_config_file <- function(config_file) { parsed_setting <- tryCatch( str2lang(dcf_values[[setting]]), error = function(e) { - stop("Malformed config setting '", setting, "':\n ", conditionMessage(e), call. = FALSE) + cli_abort(c( + i = "Malformed config setting {.field {setting}}:", + x = conditionMessage(e) + )) } ) setting_value <- withCallingHandlers( tryCatch( eval(parsed_setting), error = function(e) { - stop( - "Error from config setting '", setting, "' in '", format(conditionCall(e)), "':\n", - " ", conditionMessage(e), - call. = FALSE - ) + cli_abort(c( + "Error from config setting {.code {setting}} in {.code {format(conditionCall(e))}}:", + conditionMessage(e) + )) } ), warning = function(w) { - warning( - "Warning from config setting '", setting, "' in '", format(conditionCall(w)), "':\n", - " ", conditionMessage(w), - call. = FALSE - ) + cli_warn(c( + "Warning from config setting {.code {setting}} in {.code {format(conditionCall(w))}}:", + conditionMessage(w) + )) invokeRestart("muffleWarning") } ) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 67cd23158..0f4a7ba59 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -81,7 +81,7 @@ test_that("it gives informative errors if the config file contains errors", { withr::local_dir(withr::local_tempdir()) writeLines("a <- 1", "aaa.R") - expect_error(lint_dir(), "Error from config setting 'linters'", fixed = TRUE) + expect_error(lint_dir(), "Error from config setting `linters`", fixed = TRUE) }) test_that("rot utility works as intended", { From 56dd7e0a1e9a48863f5a5fdd92db630a76d02c63 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 6 Apr 2024 08:26:24 +0200 Subject: [PATCH 32/65] converting more errors to cli from settings --- R/lint.R | 5 ++++- R/settings.R | 5 ++++- tests/testthat/test-lint.R | 6 ++++-- tests/testthat/test-settings.R | 16 ++++++++++------ 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/R/lint.R b/R/lint.R index e588ad161..a2cd2a3b2 100644 --- a/R/lint.R +++ b/R/lint.R @@ -88,7 +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) { - stop("Linter '", linter, "' failed in ", filename, ": ", conditionMessage(cond), call. = FALSE) + cli_abort(c( + i = "Linter {.fn linter} failed in {.file {filename}}:", + x = conditionMessage(cond) + )) } ) } diff --git a/R/settings.R b/R/settings.R index f0eba02e9..49e847eb4 100644 --- a/R/settings.R +++ b/R/settings.R @@ -189,7 +189,10 @@ validate_keys <- function(config, keys, test, what) { next } if (!test(val)) { - stop("Setting '", key, "' should be ", what, ", not '", toString(val), "'.", call. = FALSE) + cli_abort(c( + i = "Setting {.code {key}} should be {.strong {what}}.", + x = "Instead, it is {.val {toString(val)}}." + )) } } } diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 1369360bf..842ebd85d 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -227,11 +227,13 @@ test_that("Linters throwing an error give a helpful error", { # we don't care much about that, so just check basename() expect_error( lint(tmp_file, linter()), - rex::rex("Linter 'linter' failed in ", anything, basename(tmp_file), ": a broken linter") + "a broken linter", + fixed = TRUE ) expect_error( lint(tmp_file, list(broken_linter = linter())), - rex::rex("Linter 'broken_linter' failed in ", anything, basename(tmp_file), ": a broken linter") + "a broken linter", + fixed = TRUE ) }) diff --git a/tests/testthat/test-settings.R b/tests/testthat/test-settings.R index 0f4a7ba59..5ad5b63c3 100644 --- a/tests/testthat/test-settings.R +++ b/tests/testthat/test-settings.R @@ -170,23 +170,27 @@ test_that("validate_config_file() detects improperly-formed settings", { writeLines(c('exclusions: list("aaa.R")', "asdf: 1"), .lintr) expect_warning(lint_dir(), "Found unused settings in config", fixed = TRUE) + encoding_error_msg <- "Setting `encoding` should be a character string" + writeLines("encoding: FALSE", .lintr) - expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'FALSE'", fixed = TRUE) + expect_error(lint_dir(), encoding_error_msg, fixed = TRUE) writeLines("encoding: NA_character_", .lintr) - expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'NA'", fixed = TRUE) + expect_error(lint_dir(), encoding_error_msg, fixed = TRUE) writeLines('encoding: c("a", "b")', .lintr) - expect_error(lint_dir(), "Setting 'encoding' should be a character string, not 'a, b'") + expect_error(lint_dir(), encoding_error_msg, fixed = TRUE) + + exclude_error_msg <- "Setting `exclude` should be a single regular expression" writeLines("exclude: FALSE", .lintr) - expect_error(lint_dir(), "Setting 'exclude' should be a single regular expression, not 'FALSE'", fixed = TRUE) + expect_error(lint_dir(), exclude_error_msg, 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) + expect_error(lint_dir(), exclude_error_msg, fixed = TRUE) writeLines('exclude: "("', .lintr) - expect_error(lint_dir(), "Setting 'exclude' should be a single regular expression, not '('", fixed = TRUE) + expect_error(lint_dir(), exclude_error_msg, fixed = TRUE) writeLines("linters: list(1)", .lintr) expect_error(lint_dir(), "Setting `linters` should be a list of linters", fixed = TRUE) From eaf754ed5921f2a2c998b667f4e5b625829b0a52 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:35:31 +0200 Subject: [PATCH 33/65] rest of it --- R/expect_lint.R | 8 ++++---- R/extract.R | 2 +- R/lint.R | 8 ++++---- R/object_usage_linter.R | 9 ++++----- R/use_lintr.R | 2 +- R/utils.R | 10 +++++----- R/with.R | 11 +++++------ tests/testthat/test-expect_lint.R | 2 +- tests/testthat/test-lint.R | 20 +++++++------------- tests/testthat/test-lint_dir.R | 2 +- tests/testthat/test-with.R | 3 ++- 11 files changed, 35 insertions(+), 42 deletions(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index ec4ca9abb..749ffeda3 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -90,10 +90,10 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { itr <<- itr + 1L lapply(names(check), function(field) { if (!field %in% lint_fields) { - stop(sprintf( - "check #%d had an invalid field: \"%s\"\nValid fields are: %s\n", - itr, field, toString(lint_fields) - ), call. = FALSE) + cli_abort(c( + x = "Check {.val {itr}} has an invalid field: {.field {field}}.", + i = "Valid fields are: {.field {toString(lint_fields)}}." + )) } check <- check[[field]] value <- lint[[field]] diff --git a/R/extract.R b/R/extract.R index 3f35e09a4..5f64cba90 100644 --- a/R/extract.R +++ b/R/extract.R @@ -119,7 +119,7 @@ filter_chunk_end_positions <- function(starts, ends) { bad_end_indexes <- grep("starts", names(code_ends), fixed = TRUE) if (length(bad_end_indexes) > 0L) { bad_start_positions <- positions[code_start_indexes[bad_end_indexes]] - # This error message is formatted like a parse error + # This error message is formatted like a parse error; don't use {cli} stop(sprintf( ":%1$d:1: Missing chunk end for chunk (maybe starting at line %1$d).\n", bad_start_positions[1L] diff --git a/R/lint.R b/R/lint.R index a2cd2a3b2..d6630c078 100644 --- a/R/lint.R +++ b/R/lint.R @@ -340,10 +340,10 @@ validate_linter_object <- function(linter, name) { return(linter) } if (!is.function(linter)) { - stop(gettextf( - "Expected '%s' to be a function of class 'linter', not a %s of class '%s'", - name, typeof(linter), class(linter)[[1L]] - ), call. = FALSE) + cli_abort(c( + i = "Expected {.fn {name}} to be a function of class {.cls linter}.", + x = "Instead, it is of class {.cls {class(linter)[[1L]]}}." + )) } if (is_linter_factory(linter)) { what <- "Passing linters as variables" diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 21cb1151e..94e17667f 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -210,11 +210,10 @@ parse_check_usage <- function(expression, is_missing <- is.na(res$message) if (any(is_missing)) { # TODO(#2474): Remove this. - warning( - "Possible bug in lintr: Couldn't parse usage message ", sQuote(vals[is_missing][[1L]]), ". ", - "Ignoring ", sum(is_missing), " usage warnings. Please report an issue at https://github.com/r-lib/lintr/issues.", - call. = FALSE - ) + cli_warn(c( + i = "Possible bug in lintr: Couldn't parse usage message {sQuote(vals[is_missing][[1L]])}.", + x = "Ignoring {.val {sum(is_missing)}} usage warnings. Please report an issue at {.url https://github.com/r-lib/lintr/issues}." + )) } # nocov end res <- res[!is_missing, ] diff --git a/R/use_lintr.R b/R/use_lintr.R index d72711b5e..16b33f279 100644 --- a/R/use_lintr.R +++ b/R/use_lintr.R @@ -27,7 +27,7 @@ use_lintr <- function(path = ".", type = c("tidyverse", "full")) { config_file <- normalizePath(file.path(path, lintr_option("linter_file")), mustWork = FALSE) if (file.exists(config_file)) { - stop("Found an existing configuration file at '", config_file, "'.", call. = FALSE) + cli_abort("Found an existing configuration file at {.file {config_file}}.") } type <- match.arg(type) the_config <- switch( diff --git a/R/utils.R b/R/utils.R index e7d2e9eb4..340a9064c 100644 --- a/R/utils.R +++ b/R/utils.R @@ -287,9 +287,9 @@ check_dots <- function(dot_names, ref_calls, ref_help = as.character(sys.call(-1 if (all(is_valid)) { return(invisible()) } - stop( - "Found unknown arguments in ...: ", toString(dot_names[!is_valid]), ".\n", - "Check for typos and see ?", ref_help, " for valid arguments.", - call. = FALSE - ) + invalid_args <- toString(dot_names[!is_valid]) # nolint: object_usage_linter. + cli_abort(c( + x = "Found unknown arguments in `...`: {invalid_args}.", + i = "Check for typos and see ?{ref_help} for valid arguments." + )) } diff --git a/R/with.R b/R/with.R index 6bc641337..65778c891 100644 --- a/R/with.R +++ b/R/with.R @@ -101,12 +101,11 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { - missing_linters <- setdiff(available$linter, ns_exports) - stop( - "Linters ", glue_collapse(sQuote(missing_linters), sep = ", ", last = " and "), - " advertised by `available_linters()` but not exported by package ", package, ".", - call. = FALSE - ) + missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. + 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}}." + )) } linter_factories <- mget(available$linter, envir = pkg_ns) linters <- Map( diff --git a/tests/testthat/test-expect_lint.R b/tests/testthat/test-expect_lint.R index 622882884..74ae79047 100644 --- a/tests/testthat/test-expect_lint.R +++ b/tests/testthat/test-expect_lint.R @@ -23,7 +23,7 @@ test_that("single check", { expect_success(expect_lint("a=1", c(message = lint_msg, line_number = 1L), linter)) expect_failure(expect_lint("a=1", c(line_number = 2L, message = lint_msg), linter)) - expect_error(expect_lint("a=1", c(message = lint_msg, lineXXX = 1L), linter), "invalid field") + expect_error(expect_lint("a=1", c(message = lint_msg, lineXXX = 1L), linter), "Check 1 has an invalid field: lineXXX") expect_failure(expect_lint("foo ()", list(ranges = list(c(2L, 2L))), function_left_parentheses_linter())) expect_success(expect_lint("\t1", list(ranges = list(c(1L, 1L))), whitespace_linter())) diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 842ebd85d..4840a7939 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -216,27 +216,21 @@ test_that("old compatibility usage errors", { expect_error( lint("a <- 1\n", linters = "equals_na_linter"), - regexp = rex::rex("Expected '", anything, "' to be a function of class 'linter'") + regexp = "Expected `linters()` to be a function of class ", + fixed = TRUE ) }) test_that("Linters throwing an error give a helpful error", { tmp_file <- withr::local_tempfile(lines = "a <- 1") - linter <- function() Linter(function(source_expression) stop("a broken linter", call. = FALSE)) + lintr_error_msg <- "a broken linter" + linter <- function() Linter(function(source_expression) stop(lintr_error_msg, call. = FALSE)) # NB: Some systems/setups may use e.g. symlinked files when creating under tempfile(); # we don't care much about that, so just check basename() - expect_error( - lint(tmp_file, linter()), - "a broken linter", - fixed = TRUE - ) - expect_error( - lint(tmp_file, list(broken_linter = linter())), - "a broken linter", - fixed = TRUE - ) + expect_error(lint(tmp_file, linter()), lintr_error_msg, fixed = TRUE) + expect_error(lint(tmp_file, list(broken_linter = linter())), lintr_error_msg, fixed = TRUE) }) test_that("typo in argument name gives helpful error", { - expect_error(lint("xxx", litners = identity), "Found unknown arguments in [.][.][.].*[?]lint ") + expect_error(lint("xxx", litners = identity), "Found unknown arguments in `...`: litners") }) diff --git a/tests/testthat/test-lint_dir.R b/tests/testthat/test-lint_dir.R index b755dea36..9b119cbdb 100644 --- a/tests/testthat/test-lint_dir.R +++ b/tests/testthat/test-lint_dir.R @@ -100,7 +100,7 @@ test_that("lint_dir works with specific linters without specifying other argumen }) test_that("typo in argument name gives helpful error", { - expect_error(lint_dir(litners = identity), "Found unknown arguments in [.][.][.].*[?]lint_dir ") + expect_error(lint_dir(litners = identity), "Found unknown arguments in `...`: litners") }) test_that("linting empty directory passes", { diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index f1d6b7389..c0be8d716 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -34,7 +34,8 @@ test_that("linters_with_tags() verifies the output of available_linters()", { ) expect_error( linters_with_tags(NULL), - "'fake_linter' and 'very_fake_linter'" + "Linters `fake_linter()` and `very_fake_linter()` are advertised by `available_linters()`", + fixed = TRUE ) }) From eed787fc8e3acc9e8b88648fcf498b843c541160 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:37:38 +0200 Subject: [PATCH 34/65] fix lint --- R/lint.R | 2 +- tests/testthat/test-Lint-builder.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/lint.R b/R/lint.R index d6630c078..6f9804677 100644 --- a/R/lint.R +++ b/R/lint.R @@ -466,7 +466,7 @@ check_ranges <- function(ranges, max_col) { } else if (!is_valid_range(range, max_col)) { cli_abort(c( x = "Invalid range specified.", - i = "All entries in {.arg ranges} must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 ({.val {max_col}})." + i = "Argument {.arg ranges} must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 ({.val {max_col}})." )) } } diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index 61d05f9fe..aad05a391 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -27,7 +27,7 @@ test_that("Lint() errors on invalid input", { ) expect_error( Lint("dummy.R", line = dummy_line, ranges = list(c(1L, 2L), c(1L, 5L))), - "All entries in `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).", + "Argument `ranges` must satisfy 0 <= range[1L] <= range[2L] <= nchar(line) + 1 (4).", fixed = TRUE ) }) From d9a8356273a67cda983db94fe5f742a66da3f33b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:38:53 +0200 Subject: [PATCH 35/65] Update NEWS.md --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index ed14e274f..3278ef0d3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -28,7 +28,7 @@ ## New and improved features -* All user-facing messages are now prepared using the [`{cli}`](https://github.com/r-lib/cli) package (#2418, @IndrajeetPatil). +* All user-facing messages are now prepared using the [`{cli}`](https://github.com/r-lib/cli) package (#2418, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent. * More helpful errors for invalid configs (#2253, @MichaelChirico). * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). From 0a4d970ec58b29462262c8cb1e0d8d5cb6ba743b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:42:00 +0200 Subject: [PATCH 36/65] update checks dots error message --- R/utils.R | 2 +- tests/testthat/test-lint.R | 2 +- tests/testthat/test-lint_dir.R | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/utils.R b/R/utils.R index 340a9064c..f444e3ebc 100644 --- a/R/utils.R +++ b/R/utils.R @@ -289,7 +289,7 @@ check_dots <- function(dot_names, ref_calls, ref_help = as.character(sys.call(-1 } invalid_args <- toString(dot_names[!is_valid]) # nolint: object_usage_linter. cli_abort(c( - x = "Found unknown arguments in `...`: {invalid_args}.", + x = "Found unknown arguments in `...`: {.arg {invalid_args}}.", i = "Check for typos and see ?{ref_help} for valid arguments." )) } diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 4840a7939..6698aeae4 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -232,5 +232,5 @@ test_that("Linters throwing an error give a helpful error", { }) test_that("typo in argument name gives helpful error", { - expect_error(lint("xxx", litners = identity), "Found unknown arguments in `...`: litners") + expect_error(lint("xxx", litners = identity), "Found unknown arguments in `...`: `litners`") }) diff --git a/tests/testthat/test-lint_dir.R b/tests/testthat/test-lint_dir.R index 9b119cbdb..24345ecd2 100644 --- a/tests/testthat/test-lint_dir.R +++ b/tests/testthat/test-lint_dir.R @@ -100,7 +100,7 @@ test_that("lint_dir works with specific linters without specifying other argumen }) test_that("typo in argument name gives helpful error", { - expect_error(lint_dir(litners = identity), "Found unknown arguments in `...`: litners") + expect_error(lint_dir(litners = identity), "Found unknown arguments in `...`: `litners`") }) test_that("linting empty directory passes", { From 561bb194f604e705cdb498f4ebe43c0efa1ebdf2 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:47:33 +0200 Subject: [PATCH 37/65] fix lint --- R/object_usage_linter.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 94e17667f..6f44a5a43 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -210,9 +210,10 @@ parse_check_usage <- function(expression, is_missing <- is.na(res$message) if (any(is_missing)) { # TODO(#2474): Remove this. + missing_msg <- vals[is_missing][[1L]] # nolint: object_usage_linter. cli_warn(c( - i = "Possible bug in lintr: Couldn't parse usage message {sQuote(vals[is_missing][[1L]])}.", - x = "Ignoring {.val {sum(is_missing)}} usage warnings. Please report an issue at {.url https://github.com/r-lib/lintr/issues}." + x = "Couldn't parse usage message {sQuote(missing_msg)}. Ignoring {.val {sum(is_missing)}} usage warnings.", + i = "Please report a possible bug at {.url https://github.com/r-lib/lintr/issues}." )) } # nocov end From 766f1bb6938bd958906f75382b258ac7e138de1c Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 09:52:52 +0200 Subject: [PATCH 38/65] Update backport_linter.R --- R/backport_linter.R | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/R/backport_linter.R b/R/backport_linter.R index 257e76191..5f965e5c4 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -94,9 +94,9 @@ normalize_r_version <- function(r_version) { if (!r_version %in% version_names) { # This can only trip if e.g. oldrel-99 is requested cli_abort(c( - "{.var r_version} is not valid:", - i = "It must be a version number or one of {toString(sQuote(version_names))}", - x = "You entered {toString(sQuote(r_version))} instead" + "{.arg r_version} is not valid:", + i = "It must be a version number or one of {toString(sQuote(version_names))}.", + x = "You entered {toString(sQuote(r_version))} instead." )) } requested_version <- minor_versions[match(r_version, table = version_names)] @@ -109,12 +109,12 @@ normalize_r_version <- function(r_version) { } else if (is.character(r_version)) { r_version <- R_system_version(r_version, strict = TRUE) } else if (!inherits(r_version, "R_system_version")) { - cli_abort("{.var r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") + cli_abort("{.arg r_version} must be an R version number, returned by {.fun R_system_version}, or a string.") } if (r_version < "3.0.0") { cli_warn(c( x = "Depending on an R version older than {.val 3.0.0} is not recommended.", - i = "Resetting {.var r_version} to {.val 3.0.0}." + i = "Resetting {.arg r_version} to {.val 3.0.0}." )) r_version <- R_system_version("3.0.0") } From 1474bd0fb20ee54614e6cd987872af4a03ef357d Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 10:32:39 +0200 Subject: [PATCH 39/65] another round of refinement --- R/cache.R | 2 +- R/exclude.R | 6 +++--- R/expect_lint.R | 2 +- R/lint.R | 2 +- tests/testthat/test-Lint-builder.R | 4 ++-- tests/testthat/test-normalize_exclusions.R | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/cache.R b/R/cache.R index 2bc3ae4d0..e2a63bb4b 100644 --- a/R/cache.R +++ b/R/cache.R @@ -54,7 +54,7 @@ load_cache <- function(file, path = NULL) { }, error = function(e) { cli_warn(c( - x = "Could not load cache file {.file {file}}:\n", + x = "Could not load cache file {.file {file}}:", i = conditionMessage(e) )) } diff --git a/R/exclude.R b/R/exclude.R index 0b9354860..09ba34498 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -131,7 +131,7 @@ parse_exclusions <- function(file, ends_msg <- line_info(ends, type = "end") # nolint: object_usage_linter. cli_abort(c( i = "Equal number of line starts and ends expected for exclusion from linting.", - x = "{.file {file}} has {starts_msg}, but only {ends_msg}." + x = "{.file {file}} has {.strong {starts_msg}}, but only {.strong {ends_msg}}." )) } @@ -260,7 +260,7 @@ normalize_exclusions <- function(x, normalize_path = TRUE, if (any(bad)) { cli_abort(c( - i = "Full file exclusions must be character vectors of length 1.", + i = "Full file exclusions must be {.cls character} vectors of length 1.", x = "Items at following indexes are not: {.val {bad_idx}}." )) } @@ -279,7 +279,7 @@ normalize_exclusions <- function(x, normalize_path = TRUE, if (any(bad)) { cli_abort(c( - i = "Full line exclusions must be numeric or integer vectors.", + i = "Full line exclusions must be {.cls numeric} or {.cls integer} vectors.", x = "Items at following indexes are not: {.val {bad_idx}}." )) } diff --git a/R/expect_lint.R b/R/expect_lint.R index 749ffeda3..d219ce1e9 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -110,7 +110,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!is.logical(ok)) { cli_abort(c( x = "Invalid regex result. Did you mistakenly have a capture group in the regex?", - i = "Be sure to escape parenthesis with `[]`" + i = "Be sure to escape parenthesis with `[]`." )) } testthat::expect(ok, msg) diff --git a/R/lint.R b/R/lint.R index 6f9804677..bc534f8b1 100644 --- a/R/lint.R +++ b/R/lint.R @@ -462,7 +462,7 @@ check_ranges <- function(ranges, max_col) { for (range in ranges) { if (!is_number(range, 2L)) { - cli_abort("{.arg ranges} must only contain length 2 integer vectors without {.code NA}s.") + cli_abort("{.arg ranges} must only contain {.cls integer} vectors of length 2 without {.code NA}s.") } else if (!is_valid_range(range, max_col)) { cli_abort(c( x = "Invalid range specified.", diff --git a/tests/testthat/test-Lint-builder.R b/tests/testthat/test-Lint-builder.R index aad05a391..d21c33f22 100644 --- a/tests/testthat/test-Lint-builder.R +++ b/tests/testthat/test-Lint-builder.R @@ -17,12 +17,12 @@ test_that("Lint() errors on invalid input", { ) expect_error( Lint("dummy.R", ranges = list(1L)), - "`ranges` must only contain length 2 integer vectors without `NA`s.", + "`ranges` must only contain vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( Lint("dummy.R", ranges = list(c(1L, NA_integer_))), - "`ranges` must only contain length 2 integer vectors without `NA`s.", + "`ranges` must only contain vectors of length 2 without `NA`s.", fixed = TRUE ) expect_error( diff --git a/tests/testthat/test-normalize_exclusions.R b/tests/testthat/test-normalize_exclusions.R index 832fa8130..f2b505e8b 100644 --- a/tests/testthat/test-normalize_exclusions.R +++ b/tests/testthat/test-normalize_exclusions.R @@ -143,11 +143,11 @@ test_that("it normalizes file paths, removing non-existing files", { }) test_that("it errors for invalid specifications", { - msg_full_files <- "Full file exclusions must be character vectors of length 1." + msg_full_files <- "Full file exclusions must be vectors of length 1." expect_error(lintr:::normalize_exclusions(2L), msg_full_files) expect_error(lintr:::normalize_exclusions(list("a.R", 2L)), msg_full_files) expect_error(lintr:::normalize_exclusions(list(a.R = Inf, 2L)), msg_full_files) - msg_full_lines <- "Full line exclusions must be numeric or integer vectors." + msg_full_lines <- "Full line exclusions must be or vectors." expect_error(lintr:::normalize_exclusions(list(a.R = "Inf")), msg_full_lines) }) From 7e1ac5a82ebe12f0d65333847745b19271a09e5c Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 10:53:25 +0200 Subject: [PATCH 40/65] use more classes --- R/linter_tags.R | 6 +++--- R/path_utils.R | 2 +- R/semicolon_linter.R | 2 +- R/with.R | 2 +- R/xml_nodes_to_lints.R | 4 ++-- tests/testthat/test-linter_tags.R | 6 +++--- tests/testthat/test-semicolon_linter.R | 2 +- tests/testthat/test-with.R | 2 +- tests/testthat/test-xml_nodes_to_lints.R | 2 +- 9 files changed, 14 insertions(+), 14 deletions(-) diff --git a/R/linter_tags.R b/R/linter_tags.R index 69db2b8e9..c4992dc0c 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -49,13 +49,13 @@ #' @export available_linters <- function(packages = "lintr", tags = NULL, exclude_tags = "deprecated") { if (!is.character(packages)) { - cli_abort("{.arg packages} must be a character vector.") + cli_abort("{.arg packages} must be a {.cls character} vector.") } if (!is.null(tags) && !is.character(tags)) { - cli_abort("{.arg tags} must be a character vector.") + cli_abort("{.arg tags} must be a {.cls character} vector.") } if (!is.null(exclude_tags) && !is.character(exclude_tags)) { - cli_abort("{.arg exclude_tags} must be a character vector.") + cli_abort("{.arg exclude_tags} must be a {.cls character} vector.") } # any tags specified explicitly will not be excluded (#1959) diff --git a/R/path_utils.R b/R/path_utils.R index a9a8f3a90..2ff8de9ec 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -108,7 +108,7 @@ is_valid_long_path <- function(path, lax = FALSE) { split_paths <- function(path, sep = "/|\\\\") { if (!is.character(path)) { - cli_abort("Argument {.arg path} should be a character vector.") + cli_abort("Argument {.arg path} should be a {.cls character} vector.") } if (!is.character(sep) || length(sep) != 1L || !nzchar(sep)) { cli_abort("Argument {.arg sep} should be a non-empty regular expression character string.") diff --git a/R/semicolon_linter.R b/R/semicolon_linter.R index febef1d8e..1e0297917 100644 --- a/R/semicolon_linter.R +++ b/R/semicolon_linter.R @@ -65,7 +65,7 @@ semicolon_linter <- function(allow_compound = FALSE, allow_trailing = FALSE) { if (allow_compound && allow_trailing) { cli_abort(c( - x = "At least one of {.arg allow_compound} or {.arg allow_trailing} must be FALSE.", + x = "At least one of {.arg allow_compound} or {.arg allow_trailing} must be `FALSE`.", i = "No lints can be generated otherwise." )) } else if (allow_compound && !allow_trailing) { diff --git a/R/with.R b/R/with.R index 65778c891..340a72b3b 100644 --- a/R/with.R +++ b/R/with.R @@ -91,7 +91,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 character vector, or {.code NULL}.") + cli_abort("{.arg tags} must be a {.cls character} vector, or {.code NULL}.") } tagged_linters <- list() diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index bb4126ff8..bad119aa1 100644 --- a/R/xml_nodes_to_lints.R +++ b/R/xml_nodes_to_lints.R @@ -54,8 +54,8 @@ xml_nodes_to_lints <- function(xml, source_expression, lint_message, return(lints) } else if (!is_node(xml)) { cli_abort(c( - x = "Expected an xml_nodeset, a list of xml_nodes, or an xml_node.", - i = "Instead got an object of class(es): {toString(class(xml))}" + 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 {toString(class(xml))}}" )) } type <- match.arg(type, c("style", "warning", "error")) diff --git a/tests/testthat/test-linter_tags.R b/tests/testthat/test-linter_tags.R index d3b1a6fc3..985cdb30d 100644 --- a/tests/testthat/test-linter_tags.R +++ b/tests/testthat/test-linter_tags.R @@ -1,7 +1,7 @@ test_that("input validation for available_linters works as expected", { - expect_error(available_linters(1L), "`packages` must be a character vector.") - expect_error(available_linters(tags = 1L), "`tags` must be a character vector.") - expect_error(available_linters(exclude_tags = 1L), "`exclude_tags` must be a character vector.") + expect_error(available_linters(1L), "`packages` must be a vector.") + expect_error(available_linters(tags = 1L), "`tags` must be a vector.") + expect_error(available_linters(exclude_tags = 1L), "`exclude_tags` must be a vector.") }) test_that("validate_linter_db works as expected", { diff --git a/tests/testthat/test-semicolon_linter.R b/tests/testthat/test-semicolon_linter.R index 674634699..8a72da509 100644 --- a/tests/testthat/test-semicolon_linter.R +++ b/tests/testthat/test-semicolon_linter.R @@ -99,7 +99,7 @@ test_that("Trailing semicolons only", { test_that("Compound semicolons only", { expect_error( lint(text = "a <- 1;", linters = semicolon_linter(allow_trailing = TRUE, allow_compound = TRUE)), - "At least one of `allow_compound` or `allow_trailing` must be FALSE", + "At least one of `allow_compound` or `allow_trailing` must be `FALSE`", fixed = TRUE ) }) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index c0be8d716..9508a5e73 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -5,7 +5,7 @@ test_that("modify_defaults produces error with missing or incorrect defaults", { }) test_that("linters_with_tags produces error with incorrect tags", { - expect_error(linters_with_tags(1L:4L), "`tags` must be a character vector, or `NULL`.", fixed = TRUE) + expect_error(linters_with_tags(1L:4L), "`tags` must be a vector, or `NULL`.", fixed = TRUE) }) test_that("linters_with_defaults works as expected with unnamed args", { diff --git a/tests/testthat/test-xml_nodes_to_lints.R b/tests/testthat/test-xml_nodes_to_lints.R index c609893d2..b5ef060ac 100644 --- a/tests/testthat/test-xml_nodes_to_lints.R +++ b/tests/testthat/test-xml_nodes_to_lints.R @@ -156,5 +156,5 @@ test_that("it doesn't produce invalid lints", { }) test_that("erroneous input errors", { - expect_error(xml_nodes_to_lints(1L), "Expected an xml_nodeset", fixed = TRUE) + expect_error(xml_nodes_to_lints(1L), "Expected an ", fixed = TRUE) }) From 44729871b991ee9cbc37323dc8eb0d96f06e301a Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 7 Apr 2024 13:02:46 +0200 Subject: [PATCH 41/65] address review comments --- R/addins.R | 3 ++- R/backport_linter.R | 4 ++-- R/exclude.R | 2 +- tests/testthat/test-exclusions.R | 4 ++-- tests/testthat/test-parse_exclusions.R | 4 ++-- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/R/addins.R b/R/addins.R index 57c54f86e..98f8f5fa0 100644 --- a/R/addins.R +++ b/R/addins.R @@ -5,7 +5,8 @@ addin_lint <- function() { } filename <- rstudioapi::getSourceEditorContext() if (filename$path == "") { - return("Current source has no path. Please save before continuing.") + cli_warn("Current source has no path. Please save before continuing.") + return(list()) } lint(filename$path) diff --git a/R/backport_linter.R b/R/backport_linter.R index 5f965e5c4..711f1a506 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -95,8 +95,8 @@ normalize_r_version <- function(r_version) { # This can only trip if e.g. oldrel-99 is requested cli_abort(c( "{.arg r_version} is not valid:", - i = "It must be a version number or one of {toString(sQuote(version_names))}.", - x = "You entered {toString(sQuote(r_version))} instead." + i = "It must be a version number or one of {sQuote(version_names)}.", + x = "You entered {sQuote(r_version)} instead." )) } requested_version <- minor_versions[match(r_version, table = version_names)] diff --git a/R/exclude.R b/R/exclude.R index 09ba34498..a155b1d4c 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -131,7 +131,7 @@ parse_exclusions <- function(file, ends_msg <- line_info(ends, type = "end") # nolint: object_usage_linter. cli_abort(c( i = "Equal number of line starts and ends expected for exclusion from linting.", - x = "{.file {file}} has {.strong {starts_msg}}, but only {.strong {ends_msg}}." + x = "{.file {file}} has {.strong {starts_msg}} and {.strong {ends_msg}}." )) } diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index d8d810928..d4e8dd5c6 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -41,7 +41,7 @@ test_that("it gives the expected error message when there is only one start but expect_error( lintr:::parse_exclusions("dummy_projects/project/one_start_no_end.R"), - "has 1 range start (line 3), but only 0 range ends", + "has 1 range start (line 3) and 0 range ends", fixed = TRUE ) }) @@ -51,7 +51,7 @@ test_that("it gives the expected error message when there is mismatch between mu expect_error( lintr:::parse_exclusions("dummy_projects/project/mismatched_starts_ends.R"), - "has 3 range starts (lines 3, 7, 11), but only 2 range ends (lines 1, 9)", + "has 3 range starts (lines 3, 7, 11) and 2 range ends (lines 1, 9)", fixed = TRUE ) }) diff --git a/tests/testthat/test-parse_exclusions.R b/tests/testthat/test-parse_exclusions.R index a6d0e34a6..479d57f97 100644 --- a/tests/testthat/test-parse_exclusions.R +++ b/tests/testthat/test-parse_exclusions.R @@ -150,7 +150,7 @@ test_that("it throws an error if start and end are unpaired", { a test ")) - expect_error(lintr:::parse_exclusions(t1), "but only") + expect_error(lintr:::parse_exclusions(t1), "and") t2 <- withr::local_tempfile(lines = trim_some(" @@ -159,5 +159,5 @@ test_that("it throws an error if start and end are unpaired", { a #TeSt_NoLiNt_EnD test ")) - expect_error(lintr:::parse_exclusions(t2), "but only") + expect_error(lintr:::parse_exclusions(t2), "and") }) From 38dffd9faccaa7eeb53a6676c5e4e53073914e3d Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 13 Apr 2024 16:08:42 +0200 Subject: [PATCH 42/65] address a few of the review comments --- NEWS.md | 2 +- R/addins.R | 2 +- R/expect_lint.R | 7 ++----- R/linter_tags.R | 2 +- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/NEWS.md b/NEWS.md index 3278ef0d3..493dd91eb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -28,7 +28,7 @@ ## New and improved features -* All user-facing messages are now prepared using the [`{cli}`](https://github.com/r-lib/cli) package (#2418, @IndrajeetPatil). All messages have been reviewed and updated to be more informative and consistent. +* 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. * More helpful errors for invalid configs (#2253, @MichaelChirico). * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). diff --git a/R/addins.R b/R/addins.R index 98f8f5fa0..90cbae257 100644 --- a/R/addins.R +++ b/R/addins.R @@ -18,7 +18,7 @@ addin_lint_package <- function() { } project <- rstudioapi::getActiveProject() if (is.null(project)) { - cli_inform("No configuration found. Using default linters.") + cli_inform("No project found, passing current directory.") project_path <- getwd() } else { project_path <- project diff --git a/R/expect_lint.R b/R/expect_lint.R index d219ce1e9..c934cead4 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -42,10 +42,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!requireNamespace("testthat", quietly = TRUE)) { # nocov start - cli_abort(c( - i = "{.fun expect_lint} is designed to work within the {.pkg testthat} testing framework.", - x = "{.pkg testthat} is not installed." - )) + cli_abort("{.fun expect_lint} is designed to work within the {.pkg testthat} testing framework, which is not installed.") # nocov end } old_lang <- set_lang(language) @@ -110,7 +107,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!is.logical(ok)) { cli_abort(c( x = "Invalid regex result. Did you mistakenly have a capture group in the regex?", - i = "Be sure to escape parenthesis with `[]`." + i = "Be sure to escape parentheses with `[]`." )) } testthat::expect(ok, msg) diff --git a/R/linter_tags.R b/R/linter_tags.R index c4992dc0c..c8cce3fec 100644 --- a/R/linter_tags.R +++ b/R/linter_tags.R @@ -118,7 +118,7 @@ validate_linter_db <- function(available, package) { # Otherwise, fallback to an empty data frame. if (!all(c("linter", "tags") %in% colnames(available))) { cli_warn(c( - i = "{.file linters.csv} must contain the columns {.str linter} and {.str tags}.", + i = "{.file linters.csv} must contain the columns {.val {c('linter', 'tags')}}.", x = "Package {.pkg package} is missing {.str {setdiff(c('linter', 'tags'), names(available))}}." )) return(FALSE) From 34e92afe1dd13b638b1659999d2783b909cba4dc Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 13 Apr 2024 16:48:09 +0200 Subject: [PATCH 43/65] not needed anymore; testthat 3.2.1 needs waldo >= 0.5 --- tests/testthat/test-with.R | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/testthat/test-with.R b/tests/testthat/test-with.R index 9508a5e73..5ab2560ad 100644 --- a/tests/testthat/test-with.R +++ b/tests/testthat/test-with.R @@ -42,7 +42,6 @@ test_that("linters_with_tags() verifies the output of available_linters()", { test_that("all default linters are tagged default", { expect_named(linters_with_defaults(), available_linters(tags = "default")$linter) - skip_if_not_installed("waldo", "0.4.0") # needs waldo#133 # covr modifies package functions causing differing deparse() results even for identical anonymous functions. # This happens because default_linters is generated at build time and thus not modifiable by covr, whereas # linters_with_tags() constructs the linters at runtime. From 7e5865b4e6fb4dc11cad9f877ed2cc0d6a57702c Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sun, 14 Apr 2024 07:50:07 +0200 Subject: [PATCH 44/65] use internal argument for bug reports URL --- R/namespace_linter.R | 5 +---- R/tree_utils.R | 11 +++++------ R/utils.R | 4 ++++ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/namespace_linter.R b/R/namespace_linter.R index 4f726420a..c64a39bfe 100644 --- a/R/namespace_linter.R +++ b/R/namespace_linter.R @@ -84,10 +84,7 @@ namespace_linter <- function(check_exports = TRUE, check_nonexports = TRUE) { # nocov start if (any(failed_namespace)) { - cli_abort(c( - x = "Failed to retrieve namespaces for one or more of the packages used with `::` or `:::`. ", - i = "Please report the issue at {.url https://github.com/r-lib/lintr/issues}." - )) + cli_abort_internal("Failed to retrieve namespaces for one or more of the packages used with `::` or `:::`. ") } # nocov end diff --git a/R/tree_utils.R b/R/tree_utils.R index d895124e0..6da9333e1 100644 --- a/R/tree_utils.R +++ b/R/tree_utils.R @@ -9,12 +9,11 @@ generate_top_level_map <- function(pc) { while ((prev_length <- length(i_not_assigned)) > 0L) { # nolint: implicit_assignment_linter. TODO(#2015): remove this. tl_parent[i_not_assigned] <- pc$parent[match(tl_parent[i_not_assigned], pc$id)] i_not_assigned <- which(!tl_parent %in% tl_ids) - if (length(i_not_assigned) >= prev_length) { # nocov start - cli_abort(c( - x = "Logical error: unassigned set did not shrink.", - i = "Check file syntax and please report as a {.pkg lintr} bug." - )) - } # nocov end + # nocov start + if (length(i_not_assigned) >= prev_length) { + cli_abort("Logical error: unassigned set did not shrink. Check file syntax.") + } + # nocov end } tl_parent } diff --git a/R/utils.R b/R/utils.R index f444e3ebc..540022b60 100644 --- a/R/utils.R +++ b/R/utils.R @@ -293,3 +293,7 @@ check_dots <- function(dot_names, ref_calls, ref_help = as.character(sys.call(-1 i = "Check for typos and see ?{ref_help} for valid arguments." )) } + +cli_abort_internal <- function(...) { + cli_abort(..., .internal = TRUE) +} From d2c359da363711bd361e6777156da2ba9c46dc51 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 14 Apr 2024 10:33:23 -0700 Subject: [PATCH 45/65] narrower line --- R/expect_lint.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index c934cead4..3ec705329 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -42,7 +42,9 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!requireNamespace("testthat", quietly = TRUE)) { # nocov start - cli_abort("{.fun expect_lint} is designed to work within the {.pkg testthat} testing framework, which is not installed.") + cli_abort( + "{.fun expect_lint} is designed to work within the {.pkg testthat} testing framework, which is not installed." + ) # nocov end } old_lang <- set_lang(language) From 12f17cb3c5d7da2a96b851a8bec3f78a47ac47c7 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 20 Apr 2024 09:30:12 +0200 Subject: [PATCH 46/65] favour str markup over sQuote --- NEWS.md | 5 ++++- R/backport_linter.R | 4 ++-- R/exclude.R | 2 +- R/object_usage_linter.R | 2 +- R/with.R | 2 +- 5 files changed, 9 insertions(+), 6 deletions(-) diff --git a/NEWS.md b/NEWS.md index 7989204cc..fa3e20113 100644 --- a/NEWS.md +++ b/NEWS.md @@ -29,7 +29,6 @@ ## New and improved features -* 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. * More helpful errors for invalid configs (#2253, @MichaelChirico). * `library_call_linter()` is extended + to encourage all packages to be attached with `library(symbol)`, not `library("symbol", character.only = TRUE)` or "vectorized" approaches looping over package names (part of #884, @MichaelChirico). @@ -78,6 +77,10 @@ * `object_name_linter()` and `object_length_linter()` ignore {rlang} name injection like `x |> mutate("{new_name}" := foo(col))` (#1926, @MichaelChirico). No checking is applied in such cases. {data.table} in-place assignments like `DT[, "sPoNGeBob" := "friend"]` are still eligible for lints. +## 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 3.1.2 ## New and improved features diff --git a/R/backport_linter.R b/R/backport_linter.R index 711f1a506..c8674aa6e 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -95,8 +95,8 @@ normalize_r_version <- function(r_version) { # This can only trip if e.g. oldrel-99 is requested cli_abort(c( "{.arg r_version} is not valid:", - i = "It must be a version number or one of {sQuote(version_names)}.", - x = "You entered {sQuote(r_version)} instead." + i = "It must be a version number or one of {.str {version_names}}.", + x = "You entered {.str {r_version}} instead." )) } requested_version <- minor_versions[match(r_version, table = version_names)] diff --git a/R/exclude.R b/R/exclude.R index a155b1d4c..518ae2d7a 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -205,7 +205,7 @@ add_exclusions <- function(exclusions, lines, linters_string, exclude_linter_sep if (!all(matched)) { bad <- excluded_linters[!matched] # nolint: object_usage_linter. cli_warn(c( - x = "Could not find linter{?s} named {.field {sQuote(bad)}} in the list of active linters.", + x = "Could not find linter{?s} named {.field {bad}} in the list of active linters.", i = "Make sure the linter is uniquely identified by the given name or prefix." )) } diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 6f44a5a43..64c7fcd85 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -212,7 +212,7 @@ parse_check_usage <- function(expression, # TODO(#2474): Remove this. missing_msg <- vals[is_missing][[1L]] # nolint: object_usage_linter. cli_warn(c( - x = "Couldn't parse usage message {sQuote(missing_msg)}. Ignoring {.val {sum(is_missing)}} usage warnings.", + x = "Couldn't parse usage message {.str {missing_msg}}. Ignoring {.val {sum(is_missing)}} usage warnings.", i = "Please report a possible bug at {.url https://github.com/r-lib/lintr/issues}." )) } diff --git a/R/with.R b/R/with.R index 340a72b3b..b661b1ede 100644 --- a/R/with.R +++ b/R/with.R @@ -46,7 +46,7 @@ modify_defaults <- function(defaults, ...) { bad_nms <- setdiff(nms[to_null], names(defaults)) is_are <- if (length(bad_nms) > 1L) "are" else "is" # nolint: object_usage_linter. cli_warn(c( - i = "Trying to remove {.field {sQuote(bad_nms)}}, which {is_are} not in {.arg defaults}." + i = "Trying to remove {.field {bad_nms}}, which {is_are} not in {.arg defaults}." )) } From a564a1eb4f11b4a2fbbbd4ca78e87085419651ed Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 20 Apr 2024 09:38:24 +0200 Subject: [PATCH 47/65] don't nest cli exceptions; formatting is off then --- R/expect_lint.R | 4 ++-- R/lint.R | 4 ++-- R/settings.R | 8 ++++---- R/with.R | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index 3ec705329..3abec5538 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -91,7 +91,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!field %in% lint_fields) { cli_abort(c( x = "Check {.val {itr}} has an invalid field: {.field {field}}.", - i = "Valid fields are: {.field {toString(lint_fields)}}." + i = "Valid fields are: {.field {lint_fields}}." )) } check <- check[[field]] @@ -109,7 +109,7 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!is.logical(ok)) { cli_abort(c( x = "Invalid regex result. Did you mistakenly have a capture group in the regex?", - i = "Be sure to escape parentheses with `[]`." + i = "You can match parentheses with a character class, i.e. inside `[]`." )) } testthat::expect(ok, msg) diff --git a/R/lint.R b/R/lint.R index bc534f8b1..b20fdd79c 100644 --- a/R/lint.R +++ b/R/lint.R @@ -89,8 +89,8 @@ lint <- function(filename, linters = NULL, ..., cache = FALSE, parse_settings = get_lints(expr, linter, linters[[linter]], lint_cache, source_expressions$lines), error = function(cond) { cli_abort(c( - i = "Linter {.fn linter} failed in {.file {filename}}:", - x = conditionMessage(cond) + "Linter {.fn linter} failed in {.file {filename}}:", + conditionMessage(cond) )) } ) diff --git a/R/settings.R b/R/settings.R index 49e847eb4..5db62ca6c 100644 --- a/R/settings.R +++ b/R/settings.R @@ -99,8 +99,8 @@ read_config_file <- function(config_file) { load_config <- function(file) sys.source(file, config, keep.source = FALSE, keep.parse.data = FALSE) malformed <- function(e) { cli_abort(c( - i = "Malformed config file, ensure it is valid R syntax.", - x = conditionMessage(e) + "Malformed config file, ensure it is valid R syntax.", + conditionMessage(e) )) } } else { @@ -111,8 +111,8 @@ read_config_file <- function(config_file) { str2lang(dcf_values[[setting]]), error = function(e) { cli_abort(c( - i = "Malformed config setting {.field {setting}}:", - x = conditionMessage(e) + "Malformed config setting {.field {setting}}:", + conditionMessage(e) )) } ) diff --git a/R/with.R b/R/with.R index b661b1ede..6072c8e3b 100644 --- a/R/with.R +++ b/R/with.R @@ -210,8 +210,8 @@ call_linter_factory <- function(linter_factory, linter_name, package) { error = function(e) { pkg_fn <- paste0(package, "::", linter_name) # nolint: object_name_linter cli_abort(c( - i = "Could not create linter with {.fun pkg_fn}.", - x = conditionMessage(e) + "Could not create linter with {.fun pkg_fn}.", + conditionMessage(e) )) } ) From 4edc5c932442221ca805c37a240410fbf7a000b7 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 20 Apr 2024 13:11:35 +0530 Subject: [PATCH 48/65] Update R/xp_utils.R Co-authored-by: olivroy <52606734+olivroy@users.noreply.github.com> --- R/xp_utils.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/R/xp_utils.R b/R/xp_utils.R index 6279daa34..82c11b946 100644 --- a/R/xp_utils.R +++ b/R/xp_utils.R @@ -83,10 +83,10 @@ xp_call_name <- function(expr, depth = 1L, condition = NULL) { ) is_valid_expr <- is_node(expr) || is_nodeset(expr) if (!is_valid_expr) { - cli_abort(c( - i = "Expected an {.cls xml_nodeset} or an {.cls xml_node}.", - x = "Instead, {.arg expr} is object of class: {.cls {class(expr)}}" - )) + cli_abort( + "{.arg expr} must be an {.cls xml_nodeset} or an {.cls xml_node}, \\ + not {.obj_type_friendly {expr}}." + ) } if (is.null(condition)) { From d1f1fe0b089a8082300c1c1b2eaf2e4acc75bec0 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 20 Apr 2024 09:46:51 +0200 Subject: [PATCH 49/65] fix test --- tests/testthat/test-xp_utils.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-xp_utils.R b/tests/testthat/test-xp_utils.R index 5d78cae81..2e7fcbeaa 100644 --- a/tests/testthat/test-xp_utils.R +++ b/tests/testthat/test-xp_utils.R @@ -12,7 +12,7 @@ test_that("xp_call_name works", { }) test_that("xp_call_name input validation works", { - expect_error(xp_call_name(2L), "Expected an ", fixed = TRUE) + expect_error(xp_call_name(2L), "`expr` must be an ", fixed = TRUE) xml <- xml2::read_xml("") expect_error(xp_call_name(xml, depth = -1L), "depth >= 0", fixed = TRUE) From ed551210d93ade7c74e9ecbdbcbd44ef6d0ca808 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 20 Apr 2024 10:13:44 +0200 Subject: [PATCH 50/65] use obj_type_friendly marker --- R/lint.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/lint.R b/R/lint.R index b20fdd79c..e2f5857b3 100644 --- a/R/lint.R +++ b/R/lint.R @@ -342,7 +342,7 @@ validate_linter_object <- function(linter, name) { if (!is.function(linter)) { cli_abort(c( i = "Expected {.fn {name}} to be a function of class {.cls linter}.", - x = "Instead, it is of class {.cls {class(linter)[[1L]]}}." + x = "Instead, it is {.obj_type_friendly {linter}}." )) } if (is_linter_factory(linter)) { From 500df0a129867474cf97eebeefe5a8578193623e Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 25 May 2024 08:43:38 +0200 Subject: [PATCH 51/65] Update `default_undesirable_functions()` list To include message, warning, and stop --- .lintr | 5 ++++- R/extract.R | 2 +- R/lint.R | 2 +- R/path_utils.R | 2 +- tests/testthat/test-library_call_linter.R | 10 +++++----- tests/testthat/test-lint.R | 2 +- tests/testthat/test-unnecessary_concatenation_linter.R | 8 ++++---- 7 files changed, 17 insertions(+), 14 deletions(-) diff --git a/.lintr b/.lintr index e9b2a3cb0..0542b9eee 100644 --- a/.lintr +++ b/.lintr @@ -14,7 +14,10 @@ linters: all_linters( undesirable_function_linter(modify_defaults( defaults = default_undesirable_functions, library = NULL, - options = NULL + options = NULL, + message = "use cli::cli_inform()", + warning = "use cli::cli_warn()", + stop = "use cli::cli_abort()" )), undesirable_operator_linter(modify_defaults( defaults = default_undesirable_operators, diff --git a/R/extract.R b/R/extract.R index 5f64cba90..c9aa57b70 100644 --- a/R/extract.R +++ b/R/extract.R @@ -120,7 +120,7 @@ filter_chunk_end_positions <- function(starts, ends) { if (length(bad_end_indexes) > 0L) { bad_start_positions <- positions[code_start_indexes[bad_end_indexes]] # This error message is formatted like a parse error; don't use {cli} - stop(sprintf( + stop(sprintf( # nolint: undesirable_function_linter ":%1$d:1: Missing chunk end for chunk (maybe starting at line %1$d).\n", bad_start_positions[1L] ), call. = FALSE) diff --git a/R/lint.R b/R/lint.R index 208ca40d1..48ae96e81 100644 --- a/R/lint.R +++ b/R/lint.R @@ -430,7 +430,7 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec line_number = as.integer(line_number), column_number = as.integer(column_number), type = type, - message = message, + message = message, # nolint: undesirable_function_linter line = line, ranges = ranges, linter = NA_character_ diff --git a/R/path_utils.R b/R/path_utils.R index 2ff8de9ec..dd53d4316 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -150,7 +150,7 @@ path_linter_factory <- function(path_function, message, linter, name = linter_au line_number = token[["line1"]], column_number = path_start, type = "warning", - message = message, + message = message, # nolint: undesirable_function_linter line = source_expression[["lines"]][[as.character(token[["line1"]])]], ranges = list(c(path_start, path_end)) ) diff --git a/tests/testthat/test-library_call_linter.R b/tests/testthat/test-library_call_linter.R index e7d118906..998304666 100644 --- a/tests/testthat/test-library_call_linter.R +++ b/tests/testthat/test-library_call_linter.R @@ -360,10 +360,10 @@ patrick::with_parameters_test_that( "library_call_linter blocks simple disallowed usages", { linter <- library_call_linter() - message <- sprintf("Unify consecutive calls to %s\\(\\)\\.", call) + lint_msg <- sprintf("Unify consecutive calls to %s\\(\\)\\.", call) # one test of inline usage - expect_lint(sprintf("%1$s(library(x)); %1$s(library(y))", call), message, linter) + expect_lint(sprintf("%1$s(library(x)); %1$s(library(y))", call), lint_msg, linter) expect_lint( trim_some(glue::glue(" @@ -371,7 +371,7 @@ patrick::with_parameters_test_that( {call}(library(y)) ")), - message, + lint_msg, linter ) @@ -380,7 +380,7 @@ patrick::with_parameters_test_that( {call}(require(x)) {call}(require(y)) ")), - message, + lint_msg, linter ) @@ -390,7 +390,7 @@ patrick::with_parameters_test_that( # a comment on y {call}(library(y)) ")), - message, + lint_msg, linter ) }, diff --git a/tests/testthat/test-lint.R b/tests/testthat/test-lint.R index 2536f68dc..6655f54dc 100644 --- a/tests/testthat/test-lint.R +++ b/tests/testthat/test-lint.R @@ -224,7 +224,7 @@ test_that("old compatibility usage errors", { test_that("Linters throwing an error give a helpful error", { tmp_file <- withr::local_tempfile(lines = "a <- 1") lintr_error_msg <- "a broken linter" - linter <- function() Linter(function(source_expression) stop(lintr_error_msg, call. = FALSE)) + linter <- function() Linter(function(source_expression) cli_abort(lintr_error_msg)) # NB: Some systems/setups may use e.g. symlinked files when creating under tempfile(); # we don't care much about that, so just check basename() expect_error(lint(tmp_file, linter()), lintr_error_msg, fixed = TRUE) diff --git a/tests/testthat/test-unnecessary_concatenation_linter.R b/tests/testthat/test-unnecessary_concatenation_linter.R index e7af5f46b..55abd21b0 100644 --- a/tests/testthat/test-unnecessary_concatenation_linter.R +++ b/tests/testthat/test-unnecessary_concatenation_linter.R @@ -66,16 +66,16 @@ local({ test_that("symbolic expressions are allowed, except by request", { linter <- unnecessary_concatenation_linter() linter_strict <- unnecessary_concatenation_linter(allow_single_expression = FALSE) - message <- rex::rex("Remove unnecessary c() of a constant expression.") + lint_msg <- rex::rex("Remove unnecessary c() of a constant expression.") expect_lint("c(alpha / 2)", NULL, linter) expect_lint("c(paste0('.', 1:2))", NULL, linter) expect_lint("c(DF[cond > 1, col])", NULL, linter) # allow_single_expression = FALSE turns both into lints - expect_lint("c(alpha / 2)", message, linter_strict) - expect_lint("c(paste0('.', 1:2))", message, linter_strict) - expect_lint("c(DF[cond > 1, col])", message, linter_strict) + expect_lint("c(alpha / 2)", lint_msg, linter_strict) + expect_lint("c(paste0('.', 1:2))", lint_msg, linter_strict) + expect_lint("c(DF[cond > 1, col])", lint_msg, linter_strict) }) test_that("sequences with : are linted whenever a constant is involved", { From 8284a5835357ac6b6243cd529fd0f4ab522b11de Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Sat, 25 May 2024 10:12:15 +0200 Subject: [PATCH 52/65] remove unneeded nolint directives --- .lintr | 2 +- R/lint.R | 2 +- R/path_utils.R | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.lintr b/.lintr index 0542b9eee..88ddccc95 100644 --- a/.lintr +++ b/.lintr @@ -18,7 +18,7 @@ linters: all_linters( message = "use cli::cli_inform()", warning = "use cli::cli_warn()", stop = "use cli::cli_abort()" - )), + ), symbol_is_undesirable = FALSE), undesirable_operator_linter(modify_defaults( defaults = default_undesirable_operators, `:::` = NULL, diff --git a/R/lint.R b/R/lint.R index 48ae96e81..208ca40d1 100644 --- a/R/lint.R +++ b/R/lint.R @@ -430,7 +430,7 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec line_number = as.integer(line_number), column_number = as.integer(column_number), type = type, - message = message, # nolint: undesirable_function_linter + message = message, line = line, ranges = ranges, linter = NA_character_ diff --git a/R/path_utils.R b/R/path_utils.R index dd53d4316..2ff8de9ec 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -150,7 +150,7 @@ path_linter_factory <- function(path_function, message, linter, name = linter_au line_number = token[["line1"]], column_number = path_start, type = "warning", - message = message, # nolint: undesirable_function_linter + message = message, line = source_expression[["lines"]][[as.character(token[["line1"]])]], ranges = list(c(path_start, path_end)) ) From dd38ec82e62c87219ed6819e8837b914fe1ffdbe Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 27 May 2024 20:33:57 +0200 Subject: [PATCH 53/65] Revert "remove unneeded nolint directives" This reverts commit 8284a5835357ac6b6243cd529fd0f4ab522b11de. --- .lintr | 2 +- R/lint.R | 2 +- R/path_utils.R | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.lintr b/.lintr index 88ddccc95..0542b9eee 100644 --- a/.lintr +++ b/.lintr @@ -18,7 +18,7 @@ linters: all_linters( message = "use cli::cli_inform()", warning = "use cli::cli_warn()", stop = "use cli::cli_abort()" - ), symbol_is_undesirable = FALSE), + )), undesirable_operator_linter(modify_defaults( defaults = default_undesirable_operators, `:::` = NULL, diff --git a/R/lint.R b/R/lint.R index 208ca40d1..48ae96e81 100644 --- a/R/lint.R +++ b/R/lint.R @@ -430,7 +430,7 @@ Lint <- function(filename, line_number = 1L, column_number = 1L, # nolint: objec line_number = as.integer(line_number), column_number = as.integer(column_number), type = type, - message = message, + message = message, # nolint: undesirable_function_linter line = line, ranges = ranges, linter = NA_character_ diff --git a/R/path_utils.R b/R/path_utils.R index 2ff8de9ec..dd53d4316 100644 --- a/R/path_utils.R +++ b/R/path_utils.R @@ -150,7 +150,7 @@ path_linter_factory <- function(path_function, message, linter, name = linter_au line_number = token[["line1"]], column_number = path_start, type = "warning", - message = message, + message = message, # nolint: undesirable_function_linter line = source_expression[["lines"]][[as.character(token[["line1"]])]], ranges = list(c(path_start, path_end)) ) From 1dd21fc61f45ba6b70957b2c29ed96601f0e02ca Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 27 May 2024 21:22:29 +0200 Subject: [PATCH 54/65] remove unnecessary assignments --- R/exclude.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/R/exclude.R b/R/exclude.R index 5259f98df..8e709b7da 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -259,12 +259,11 @@ normalize_exclusions <- function(x, normalize_path = TRUE, }, logical(1L) ) - bad_idx <- which(bad) # nolint: object_usage_linter. if (any(bad)) { cli_abort(c( i = "Full file exclusions must be {.cls character} vectors of length 1.", - x = "Items at following indexes are not: {.val {bad_idx}}." + x = "Items at following indexes are not: {.val {which(bad)}}." )) } # Normalize unnamed entries to list( = list(Inf), ...) @@ -278,12 +277,11 @@ normalize_exclusions <- function(x, normalize_path = TRUE, # must be integer or numeric vectors are_numeric <- vapply(x, is.numeric, logical(1L)) bad <- full_line_exclusions & !are_numeric - bad_idx <- which(bad) # nolint: object_usage_linter. if (any(bad)) { cli_abort(c( i = "Full line exclusions must be {.cls numeric} or {.cls integer} vectors.", - x = "Items at following indexes are not: {.val {bad_idx}}." + x = "Items at following indexes are not: {.val {which(bad)}}." )) } From df4bd849a4a0723b5e8e515fc460aa92f18f8b32 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 27 May 2024 21:24:54 +0200 Subject: [PATCH 55/65] nolint line length lint --- R/expect_lint.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index 5fe876ed6..7264f0e0e 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -44,8 +44,9 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!requireNamespace("testthat", quietly = TRUE)) { # nocov start + # nolint next: line_length_linter. cli_abort( - "{.fun expect_lint} and {.fun expect_no_lint} designed to work within the {.pkg testthat} testing framework, which is not installed." + "{.fun expect_lint} and {.fun expect_no_lint} are designed to work within the {.pkg testthat} testing framework, which is not installed." ) # nocov end } From 1eb3c858e50880be9a6e2f29ba52d8ec8778bccb Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 27 May 2024 21:39:58 +0200 Subject: [PATCH 56/65] Update expect_lint.R --- R/expect_lint.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index 7264f0e0e..7bc1b65fd 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -44,8 +44,8 @@ expect_lint <- function(content, checks, ..., file = NULL, language = "en") { if (!requireNamespace("testthat", quietly = TRUE)) { # nocov start - # nolint next: line_length_linter. cli_abort( + # nolint next: line_length_linter. "{.fun expect_lint} and {.fun expect_no_lint} are designed to work within the {.pkg testthat} testing framework, which is not installed." ) # nocov end From 166b472408bc88f0bf76fb08fb7e09488937e472 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Tue, 28 May 2024 08:29:52 +0200 Subject: [PATCH 57/65] Apply suggestions from code review Co-authored-by: Michael Chirico --- R/exclude.R | 4 ++-- R/with.R | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/exclude.R b/R/exclude.R index 8e709b7da..6f928a3a6 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -263,7 +263,7 @@ normalize_exclusions <- function(x, normalize_path = TRUE, if (any(bad)) { cli_abort(c( i = "Full file exclusions must be {.cls character} vectors of length 1.", - x = "Items at following indexes are not: {.val {which(bad)}}." + x = "Items at the following indices are not: {.val {which(bad)}}." )) } # Normalize unnamed entries to list( = list(Inf), ...) @@ -281,7 +281,7 @@ normalize_exclusions <- function(x, normalize_path = TRUE, if (any(bad)) { cli_abort(c( i = "Full line exclusions must be {.cls numeric} or {.cls integer} vectors.", - x = "Items at following indexes are not: {.val {which(bad)}}." + x = "Items at the following indices are not: {.val {which(bad)}}." )) } diff --git a/R/with.R b/R/with.R index 6072c8e3b..618b7bbcb 100644 --- a/R/with.R +++ b/R/with.R @@ -210,7 +210,7 @@ call_linter_factory <- function(linter_factory, linter_name, package) { error = function(e) { pkg_fn <- paste0(package, "::", linter_name) # nolint: object_name_linter cli_abort(c( - "Could not create linter with {.fun pkg_fn}.", + "Could not create linter with {.fun {package}::{linter_name}}.", conditionMessage(e) )) } From 300b63646e13b08a47d85558e7a71e54309a3586 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Tue, 28 May 2024 08:33:38 +0200 Subject: [PATCH 58/65] cli 3.4 is needed --- DESCRIPTION | 2 +- R/with.R | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 788f0b940..b22c1dd62 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -24,7 +24,7 @@ Depends: R (>= 3.6) Imports: backports (>= 1.1.7), - cli, + cli (>= 3.4.0), codetools, digest, glue, diff --git a/R/with.R b/R/with.R index 618b7bbcb..cdd7f85b2 100644 --- a/R/with.R +++ b/R/with.R @@ -208,7 +208,6 @@ call_linter_factory <- function(linter_factory, linter_name, package) { linter <- tryCatch( linter_factory(), error = function(e) { - pkg_fn <- paste0(package, "::", linter_name) # nolint: object_name_linter cli_abort(c( "Could not create linter with {.fun {package}::{linter_name}}.", conditionMessage(e) From 58119199411aae32d288bea144cd4b57dcc394ba Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 17:40:37 +0200 Subject: [PATCH 59/65] get rid of unnecessary `toString()` --- R/settings.R | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/R/settings.R b/R/settings.R index 5db62ca6c..7f18b8527 100644 --- a/R/settings.R +++ b/R/settings.R @@ -164,7 +164,7 @@ 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. - cli_warn("Found unused settings in config {.str config_file}: {toString(unused_settings)}") + cli_warn("Found unused settings in config {.str config_file}: {.field unused_settings}") } validate_regex(config, @@ -191,7 +191,7 @@ validate_keys <- function(config, keys, test, what) { if (!test(val)) { cli_abort(c( i = "Setting {.code {key}} should be {.strong {what}}.", - x = "Instead, it is {.val {toString(val)}}." + x = "Instead, it is {.field {val}}." )) } } @@ -219,7 +219,7 @@ validate_linters <- function(linters) { non_linters <- which(!is_linters) # nolint: object_usage_linter. cli_abort(c( i = "Setting {.arg linters} should be a list of linters.", - x = "Found non-linters at elements: {toString(non_linters)}." + x = "Found non-linters at elements: {.str {non_linters}}." )) } } @@ -234,10 +234,10 @@ validate_exclusions <- function(exclusions) { 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)) { - problematic_entries <- toString(which(!has_names)[!unnamed_is_string]) # nolint: object_usage_linter. + problematic_entries <- which(!has_names)[!unnamed_is_string] # nolint: object_usage_linter. cli_abort(c( i = "Unnamed entries of setting {.arg exclusions} should be strings naming files or directories.", - x = "Check exclusions: {problematic_entries}." + x = "Check exclusions: {.str {problematic_entries}}." )) } for (ii in which(has_names)) validate_named_exclusion(exclusions, ii) From c778935d64ecc83c0d16b6e5128bd68bee39f8ae Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 17:45:59 +0200 Subject: [PATCH 60/65] add TODOs to nolint directives --- R/exclude.R | 6 +++--- R/object_usage_linter.R | 2 +- R/settings.R | 6 +++--- R/undesirable_operator_linter.R | 2 +- R/utils.R | 2 +- R/with.R | 4 ++-- R/xml_nodes_to_lints.R | 2 +- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/R/exclude.R b/R/exclude.R index 6f928a3a6..f3bbc5405 100644 --- a/R/exclude.R +++ b/R/exclude.R @@ -130,8 +130,8 @@ parse_exclusions <- function(file, if (length(starts) > 0L) { if (length(starts) != length(ends)) { - starts_msg <- line_info(starts, type = "start") # nolint: object_usage_linter. - ends_msg <- line_info(ends, type = "end") # nolint: object_usage_linter. + starts_msg <- line_info(starts, type = "start") # nolint: object_usage_linter. TODO(#2252). + ends_msg <- line_info(ends, type = "end") # nolint: object_usage_linter. TODO(#2252). cli_abort(c( i = "Equal number of line starts and ends expected for exclusion from linting.", x = "{.file {file}} has {.strong {starts_msg}} and {.strong {ends_msg}}." @@ -206,7 +206,7 @@ add_exclusions <- function(exclusions, lines, linters_string, exclude_linter_sep idxs <- pmatch(excluded_linters, linter_names, duplicates.ok = TRUE) matched <- !is.na(idxs) if (!all(matched)) { - bad <- excluded_linters[!matched] # nolint: object_usage_linter. + bad <- excluded_linters[!matched] # nolint: object_usage_linter. TODO(#2252). cli_warn(c( x = "Could not find linter{?s} named {.field {bad}} in the list of active linters.", i = "Make sure the linter is uniquely identified by the given name or prefix." diff --git a/R/object_usage_linter.R b/R/object_usage_linter.R index 64c7fcd85..6b9466eff 100644 --- a/R/object_usage_linter.R +++ b/R/object_usage_linter.R @@ -210,7 +210,7 @@ parse_check_usage <- function(expression, is_missing <- is.na(res$message) if (any(is_missing)) { # TODO(#2474): Remove this. - missing_msg <- vals[is_missing][[1L]] # nolint: object_usage_linter. + missing_msg <- vals[is_missing][[1L]] # nolint: object_usage_linter. TODO(#2252). cli_warn(c( x = "Couldn't parse usage message {.str {missing_msg}}. Ignoring {.val {sum(is_missing)}} usage warnings.", i = "Please report a possible bug at {.url https://github.com/r-lib/lintr/issues}." diff --git a/R/settings.R b/R/settings.R index 7f18b8527..614d19767 100644 --- a/R/settings.R +++ b/R/settings.R @@ -163,7 +163,7 @@ read_config_file <- function(config_file) { 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. + unused_settings <- names(config)[!matched] # nolint: object_usage_linter. TODO(#2252). cli_warn("Found unused settings in config {.str config_file}: {.field unused_settings}") } @@ -216,7 +216,7 @@ validate_linters <- function(linters) { is_linters <- vapply(linters, is_linter, logical(1L)) if (!all(is_linters)) { - non_linters <- which(!is_linters) # nolint: object_usage_linter. + non_linters <- which(!is_linters) # nolint: object_usage_linter. TODO(#2252). cli_abort(c( i = "Setting {.arg linters} should be a list of linters.", x = "Found non-linters at elements: {.str {non_linters}}." @@ -234,7 +234,7 @@ validate_exclusions <- function(exclusions) { 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)) { - problematic_entries <- which(!has_names)[!unnamed_is_string] # nolint: object_usage_linter. + problematic_entries <- which(!has_names)[!unnamed_is_string] # nolint: object_usage_linter. TODO(#2252). cli_abort(c( i = "Unnamed entries of setting {.arg exclusions} should be strings naming files or directories.", x = "Check exclusions: {.str {problematic_entries}}." diff --git a/R/undesirable_operator_linter.R b/R/undesirable_operator_linter.R index 879625cdd..e3fb7186e 100644 --- a/R/undesirable_operator_linter.R +++ b/R/undesirable_operator_linter.R @@ -61,7 +61,7 @@ undesirable_operator_linter <- function(op = default_undesirable_operators) { } if (length(operator_nodes) == 0L) { - cli_abort("Did not recognize any valid operators in request for: {toString(names(op))}") + cli_abort("Did not recognize any valid operators in request for: {.str {names(op)}}") } xpath <- paste(paste0("//", operator_nodes), collapse = " | ") diff --git a/R/utils.R b/R/utils.R index af3a78543..f6e709028 100644 --- a/R/utils.R +++ b/R/utils.R @@ -264,7 +264,7 @@ check_dots <- function(dot_names, ref_calls, ref_help = as.character(sys.call(-1 if (all(is_valid)) { return(invisible()) } - invalid_args <- toString(dot_names[!is_valid]) # nolint: object_usage_linter. + invalid_args <- dot_names[!is_valid] # nolint: object_usage_linter. TODO(#2252). cli_abort(c( x = "Found unknown arguments in `...`: {.arg {invalid_args}}.", i = "Check for typos and see ?{ref_help} for valid arguments." diff --git a/R/with.R b/R/with.R index cdd7f85b2..fbd130d50 100644 --- a/R/with.R +++ b/R/with.R @@ -44,7 +44,7 @@ modify_defaults <- function(defaults, ...) { to_null <- vapply(vals, is.null, logical(1L)) if (!all(nms[to_null] %in% names(defaults))) { bad_nms <- setdiff(nms[to_null], names(defaults)) - is_are <- if (length(bad_nms) > 1L) "are" else "is" # nolint: object_usage_linter. + is_are <- if (length(bad_nms) > 1L) "are" else "is" # nolint: object_usage_linter. TODO(#2252). cli_warn(c( i = "Trying to remove {.field {bad_nms}}, which {is_are} not in {.arg defaults}." )) @@ -101,7 +101,7 @@ linters_with_tags <- function(tags, ..., packages = "lintr", exclude_tags = "dep available <- available_linters(packages = package, tags = tags, exclude_tags = exclude_tags) if (nrow(available) > 0L) { if (!all(available$linter %in% ns_exports)) { - missing_linters <- setdiff(available$linter, ns_exports) # nolint: object_usage_linter. + 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}}." diff --git a/R/xml_nodes_to_lints.R b/R/xml_nodes_to_lints.R index 4d5fdf3f0..9da6c450d 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 {toString(class(xml))}}" + i = "Instead got an object of class(es): {.cls {class(xml)}}" )) } type <- match.arg(type, c("style", "warning", "error")) From febdf0a901f90fa02a29b9ca1efdbaaefa26aca3 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 17:50:17 +0200 Subject: [PATCH 61/65] Use cli_abort_internal --- R/tree_utils.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/tree_utils.R b/R/tree_utils.R index 6da9333e1..984ab8fcc 100644 --- a/R/tree_utils.R +++ b/R/tree_utils.R @@ -11,7 +11,7 @@ generate_top_level_map <- function(pc) { i_not_assigned <- which(!tl_parent %in% tl_ids) # nocov start if (length(i_not_assigned) >= prev_length) { - cli_abort("Logical error: unassigned set did not shrink. Check file syntax.") + cli_abort_internal("Logical error: unassigned set did not shrink. Check file syntax.") } # nocov end } From 11f5ab519b5631b560115499a8221ea376b9b843 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 17:54:29 +0200 Subject: [PATCH 62/65] fix tests --- tests/testthat/test-undesirable_operator_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-undesirable_operator_linter.R b/tests/testthat/test-undesirable_operator_linter.R index a3e2aea2b..e5c65da16 100644 --- a/tests/testthat/test-undesirable_operator_linter.R +++ b/tests/testthat/test-undesirable_operator_linter.R @@ -76,12 +76,12 @@ test_that("invalid inputs fail correctly", { expect_error( undesirable_operator_linter(c("***" = NA)), - "Did not recognize any valid operators in request for: ***", + 'Did not recognize any valid operators in request for: "***"', fixed = TRUE ) expect_error( undesirable_operator_linter(c("***" = NA, "///" = NA)), - "Did not recognize any valid operators in request for: ***, ///", + 'Did not recognize any valid operators in request for: "***" and "///"', fixed = TRUE ) }) From 0a8881e0d9f45690ba216665de68badf133b897b Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 17:59:26 +0200 Subject: [PATCH 63/65] Update xp_utils.R --- R/xp_utils.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/R/xp_utils.R b/R/xp_utils.R index 82c11b946..e72bedc85 100644 --- a/R/xp_utils.R +++ b/R/xp_utils.R @@ -83,10 +83,10 @@ xp_call_name <- function(expr, depth = 1L, condition = NULL) { ) is_valid_expr <- is_node(expr) || is_nodeset(expr) if (!is_valid_expr) { - cli_abort( - "{.arg expr} must be an {.cls xml_nodeset} or an {.cls xml_node}, \\ - not {.obj_type_friendly {expr}}." - ) + cli_abort(c( + i = "{.arg expr} must be an {.cls xml_nodeset} or an {.cls xml_node}.", + x = "Instead, it is {.obj_type_friendly {expr}}." + )) } if (is.null(condition)) { From 776408d9fc5ca9641a22de4032751064153a2f3a Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Wed, 29 May 2024 18:20:57 +0200 Subject: [PATCH 64/65] use pluralization --- R/with.R | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/R/with.R b/R/with.R index fbd130d50..f627f00ea 100644 --- a/R/with.R +++ b/R/with.R @@ -43,10 +43,9 @@ modify_defaults <- function(defaults, ...) { to_null <- vapply(vals, is.null, logical(1L)) if (!all(nms[to_null] %in% names(defaults))) { - bad_nms <- setdiff(nms[to_null], names(defaults)) - is_are <- if (length(bad_nms) > 1L) "are" else "is" # nolint: object_usage_linter. TODO(#2252). + bad_nms <- setdiff(nms[to_null], names(defaults)) # nolint: object_usage_linter. TODO(#2252). cli_warn(c( - i = "Trying to remove {.field {bad_nms}}, which {is_are} not in {.arg defaults}." + i = "Trying to remove {.field {bad_nms}}, which {?is/are} not in {.arg defaults}." )) } From 9dcbbc6b3cf7662fd5e1276a6c1bf734c83b1de4 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Tue, 11 Jun 2024 18:27:48 +0200 Subject: [PATCH 65/65] use more specific error message stem --- tests/testthat/test-parse_exclusions.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-parse_exclusions.R b/tests/testthat/test-parse_exclusions.R index 479d57f97..f77ee7f7f 100644 --- a/tests/testthat/test-parse_exclusions.R +++ b/tests/testthat/test-parse_exclusions.R @@ -143,6 +143,7 @@ test_that("it ignores exclude coverage lines within start and end", { test_that("it throws an error if start and end are unpaired", { lintr:::read_settings(NULL) + error_msg_stem <- "Equal number of line starts and ends expected for exclusion from linting" t1 <- withr::local_tempfile(lines = trim_some(" this #TeSt_NoLiNt_StArT @@ -150,7 +151,7 @@ test_that("it throws an error if start and end are unpaired", { a test ")) - expect_error(lintr:::parse_exclusions(t1), "and") + expect_error(lintr:::parse_exclusions(t1), error_msg_stem) t2 <- withr::local_tempfile(lines = trim_some(" @@ -159,5 +160,5 @@ test_that("it throws an error if start and end are unpaired", { a #TeSt_NoLiNt_EnD test ")) - expect_error(lintr:::parse_exclusions(t2), "and") + expect_error(lintr:::parse_exclusions(t2), error_msg_stem) })