From 11595ed553dd0701f5381fc7b1860363b91f80c3 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 03:55:12 +0000 Subject: [PATCH 1/7] New make_linter_from_xpath --- DESCRIPTION | 1 + NAMESPACE | 1 + NEWS.md | 3 +- R/make_linter_from_xpath.R | 43 ++++++++++++++++++++ man/make_linter_from_xpath.Rd | 34 ++++++++++++++++ tests/testthat/test-make_linter_from_xpath.R | 42 +++++++++++++++++++ 6 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 R/make_linter_from_xpath.R create mode 100644 man/make_linter_from_xpath.Rd create mode 100644 tests/testthat/test-make_linter_from_xpath.R diff --git a/DESCRIPTION b/DESCRIPTION index 917946399..252af9701 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -126,6 +126,7 @@ Collate: 'lintr-package.R' 'literal_coercion_linter.R' 'make_linter_from_regex.R' + 'make_linter_from_xpath.R' 'matrix_apply_linter.R' 'methods.R' 'missing_argument_linter.R' diff --git a/NAMESPACE b/NAMESPACE index 6469aa96e..1f5a21289 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -89,6 +89,7 @@ export(lint_package) export(linters_with_defaults) export(linters_with_tags) export(literal_coercion_linter) +export(make_linter_from_xpath) export(matrix_apply_linter) export(missing_argument_linter) export(missing_package_linter) diff --git a/NEWS.md b/NEWS.md index 1fc61de4a..907848c67 100644 --- a/NEWS.md +++ b/NEWS.md @@ -12,6 +12,8 @@ ## New and improved features * New exclusion sentinel `# nolint next` to signify the next line should skip linting (#1791, @MichaelChirico). The usual rules apply for excluding specific linters, e.g. `# nolint next: assignment_linter.`. The exact string used to match a subsequent-line exclusion is controlled by the `exclude_next` config entry or R option `"lintr.exclude_next"`. +* New `xp_call_name()` helper to facilitate writing custom linters (#2023, @MichaelChirico). This helper converts a matched XPath to the R function to which it corresponds. This is useful for including the "offending" function in the lint's message. +* New `make_linter_from_xpath()` to facilitate making simple linters directly from a single XPath (#2064, @MichaelChirico). This is especially helpful for making on-the-fly/exploratory linters, but also extends to any case where the linter can be fully defined from a static lint message and single XPath. * `fixed_regex_linter()` + Is pipe-aware, in particular removing false positives arong piping into {stringr} functions like `x |> str_replace(fixed("a"), "b")` (#1811, @MichaelChirico). + Gains an option `allow_unescaped` (default `FALSE`) to toggle linting regexes not requiring any escapes or character classes (#1689, @MichaelChirico). Thus `fixed_regex_linter(allow_unescaped = TRUE)` would lint on `grepl("[$]", x)` but not on `grepl("a", x)` since the latter does not use any regex special characters. @@ -36,7 +38,6 @@ * `paste_linter()` gains detection for file paths that are better constructed with `file.path()`, e.g. `paste0(dir, "/", file)` would be better as `file.path(dir, file)` (part of #884, #2082, @MichaelChirico). What exactly gets linted here can be fine-tuned with the `allow_file_path` option (`"double_slash"` by default, with alternatives `"never"` and `"always"`). When `"always"`, these rules are ignored. When `"double_slash"`, paths appearing to construct a URL that have consecutive forward slashes (`/`) are skipped. When `"never"`, even URLs should be construced with `file.path()`. * `paste_linter()` gains detection for file paths that are better constructed with `file.path()`, e.g. `paste0(dir, "/", file)` would be better as `file.path(dir, file)` (part of #884, @MichaelChirico). * `seq_linter()` recommends `rev()` in the lint message for lints like `nrow(x):1` (#1542, @MichaelChirico). -* New `xp_call_name()` helper to facilitate writing custom linters (#2023, @MichaelChirico). This helper converts a matched XPath to the R function to which it corresponds. This is useful for including the "offending" function in the lint's message. * `function_argument_linter()` detects usage of `missing()` for the linted argument (#1546, @MichaelChirico). The simplest fix for `function_argument_linter()` lints is typically to set that argument to `NULL` by default, in which case it's usually preferable to update function logic checking `missing()` to check `is.null()` instead. * `equals_na_linter()` checks for `x %in% NA`, which is a more convoluted form of `is.na(x)` (#2088, @MichaelChirico). * `commas_linter()` gains an option `allow_trailing` (default `FALSE`) to allow trailing commas while indexing. (#2104, @MEO265) diff --git a/R/make_linter_from_xpath.R b/R/make_linter_from_xpath.R new file mode 100644 index 000000000..f8114cbef --- /dev/null +++ b/R/make_linter_from_xpath.R @@ -0,0 +1,43 @@ +#' Create a linter from an XPath +#' +#' @inheritParams xml_nodes_to_lints +#' @inheritParams is_lint_level +#' @param xpath Character string, an XPath identifying R code to lint. +#' See [xmlparsedata::xml_parse_data()] and [get_source_expressions()]. +#' +#' @examples +#' number_linter <- make_linter_from_xpath("//NUM_CONST", "This is a number.") +#' lint(text = "1 + 2", number_linter()) +#' @export +make_linter_from_xpath <- function(xpath, + lint_message, + type = c("style", "warning", "error"), + level = c("expression", "file")) { + type <- match.arg(type) + level <- match.arg(level) + + stopifnot( + "xpath should be a character string" = is.character(xpath) && length(xpath) == 1L && !is.na(xpath) + ) + + xml_key <- if (level == "expression") "xml_parsed_content" else "full_xml_parsed_content" + + function() { + Linter(function(source_expression) { + if (!is_lint_level(source_expression, level)) { + return(list()) + } + + xml <- source_expression[[xml_key]] + + expr <- xml_find_all(xml, xpath) + + xml_nodes_to_lints( + expr, + source_expression = source_expression, + lint_message = lint_message, + type = type + ) + }) + } +} diff --git a/man/make_linter_from_xpath.Rd b/man/make_linter_from_xpath.Rd new file mode 100644 index 000000000..a292b453c --- /dev/null +++ b/man/make_linter_from_xpath.Rd @@ -0,0 +1,34 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/make_linter_from_xpath.R +\name{make_linter_from_xpath} +\alias{make_linter_from_xpath} +\title{Create a linter from an XPath} +\usage{ +make_linter_from_xpath( + xpath, + lint_message, + type = c("style", "warning", "error"), + level = c("expression", "file") +) +} +\arguments{ +\item{xpath}{Character string, an XPath identifying R code to lint. +See \code{\link[xmlparsedata:xml_parse_data]{xmlparsedata::xml_parse_data()}} and \code{\link[=get_source_expressions]{get_source_expressions()}}.} + +\item{lint_message}{The message to be included as the \code{message} +to the \code{Lint} object. If \code{lint_message} is a character vector the same length as \code{xml}, +the \code{i}-th lint will be given the \code{i}-th message.} + +\item{type}{type of lint.} + +\item{level}{Which level of expression is being tested? \code{"expression"} +means an individual expression, while \code{"file"} means all expressions +in the current file are available.} +} +\description{ +Create a linter from an XPath +} +\examples{ +number_linter <- make_linter_from_xpath("//NUM_CONST", "This is a number.") +lint(text = "1 + 2", number_linter()) +} diff --git a/tests/testthat/test-make_linter_from_xpath.R b/tests/testthat/test-make_linter_from_xpath.R new file mode 100644 index 000000000..8bfad8f22 --- /dev/null +++ b/tests/testthat/test-make_linter_from_xpath.R @@ -0,0 +1,42 @@ +test_that("basic usage works", { + linter <- make_linter_from_xpath("//NUM_CONST", "Number") + expect_true(is.function(linter)) + expect_lint("1", "Number", linter()) +}) + +test_that("input validation works", { + expect_error( + make_linter_from_xpath("//NUM_CONST", "Number", type = "x"), + 'one of "style", "warning", "error"', + fixed = TRUE + ) + + expect_error( + make_linter_from_xpath("//NUM_CONST", "Number", level = "x"), + 'one of "expression", "file"', + ) + + expect_error( + make_linter_from_xpath(FALSE), + "xpath should be a character string", + fixed = TRUE + ) + + expect_error( + make_linter_from_xpath(letters), + "xpath should be a character string", + fixed = TRUE + ) + + expect_error( + make_linter_from_xpath(NA_character_), + "xpath should be a character string", + fixed = TRUE + ) + + expect_error( + make_linter_from_xpath(character()), + "xpath should be a character string", + fixed = TRUE + ) +}) From d77d6d9701263a625a0598a732f0a197a323c23a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 04:07:38 +0000 Subject: [PATCH 2/7] warning by default --- R/make_linter_from_xpath.R | 2 +- man/make_linter_from_xpath.Rd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/make_linter_from_xpath.R b/R/make_linter_from_xpath.R index f8114cbef..229028fd2 100644 --- a/R/make_linter_from_xpath.R +++ b/R/make_linter_from_xpath.R @@ -11,7 +11,7 @@ #' @export make_linter_from_xpath <- function(xpath, lint_message, - type = c("style", "warning", "error"), + type = c("warning", "style", "error"), level = c("expression", "file")) { type <- match.arg(type) level <- match.arg(level) diff --git a/man/make_linter_from_xpath.Rd b/man/make_linter_from_xpath.Rd index a292b453c..42c7177d3 100644 --- a/man/make_linter_from_xpath.Rd +++ b/man/make_linter_from_xpath.Rd @@ -7,7 +7,7 @@ make_linter_from_xpath( xpath, lint_message, - type = c("style", "warning", "error"), + type = c("warning", "style", "error"), level = c("expression", "file") ) } From 2854f2f71d88a40fac809ad8e70b88289ead0d28 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 04:13:10 +0000 Subject: [PATCH 3/7] more tests --- tests/testthat/test-make_linter_from_xpath.R | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-make_linter_from_xpath.R b/tests/testthat/test-make_linter_from_xpath.R index 8bfad8f22..3b4c7955a 100644 --- a/tests/testthat/test-make_linter_from_xpath.R +++ b/tests/testthat/test-make_linter_from_xpath.R @@ -1,13 +1,17 @@ test_that("basic usage works", { linter <- make_linter_from_xpath("//NUM_CONST", "Number") expect_true(is.function(linter)) - expect_lint("1", "Number", linter()) + expect_lint("1", list("Number", type = "warning"), linter()) + + expect_lint("'a'", "Letter", make_linter_from_xpath("//STR_CONST", "Letter")()) + expect_lint("'a'", "Letter", make_linter_from_xpath("//STR_CONST", "Letter", level = "file")()) + expect_lint("'a'", list("Letter", type = "style"), make_linter_from_xpath("//STR_CONST", "Letter", type = "style")()) }) test_that("input validation works", { expect_error( make_linter_from_xpath("//NUM_CONST", "Number", type = "x"), - 'one of "style", "warning", "error"', + 'one of "warning", "style", "error"', fixed = TRUE ) From 10e2fb88b42807b4ecd978a8ab1047fd0ea7c2ab Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 04:45:54 +0000 Subject: [PATCH 4/7] delint --- tests/testthat/test-make_linter_from_xpath.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-make_linter_from_xpath.R b/tests/testthat/test-make_linter_from_xpath.R index 3b4c7955a..78a0aae5f 100644 --- a/tests/testthat/test-make_linter_from_xpath.R +++ b/tests/testthat/test-make_linter_from_xpath.R @@ -1,6 +1,6 @@ test_that("basic usage works", { linter <- make_linter_from_xpath("//NUM_CONST", "Number") - expect_true(is.function(linter)) + expect_type(linter, "closure") expect_lint("1", list("Number", type = "warning"), linter()) expect_lint("'a'", "Letter", make_linter_from_xpath("//STR_CONST", "Letter")()) @@ -18,6 +18,7 @@ test_that("input validation works", { expect_error( make_linter_from_xpath("//NUM_CONST", "Number", level = "x"), 'one of "expression", "file"', + fixed = TRUE ) expect_error( From d74bb4337f267f7e3ac2c4d6c3928cd3babaf656 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 04:47:31 +0000 Subject: [PATCH 5/7] fix example --- R/make_linter_from_xpath.R | 2 +- man/make_linter_from_xpath.Rd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/make_linter_from_xpath.R b/R/make_linter_from_xpath.R index 229028fd2..6a758c701 100644 --- a/R/make_linter_from_xpath.R +++ b/R/make_linter_from_xpath.R @@ -7,7 +7,7 @@ #' #' @examples #' number_linter <- make_linter_from_xpath("//NUM_CONST", "This is a number.") -#' lint(text = "1 + 2", number_linter()) +#' lint(text = "1 + 2", linters = number_linter()) #' @export make_linter_from_xpath <- function(xpath, lint_message, diff --git a/man/make_linter_from_xpath.Rd b/man/make_linter_from_xpath.Rd index 42c7177d3..ec935ef61 100644 --- a/man/make_linter_from_xpath.Rd +++ b/man/make_linter_from_xpath.Rd @@ -30,5 +30,5 @@ Create a linter from an XPath } \examples{ number_linter <- make_linter_from_xpath("//NUM_CONST", "This is a number.") -lint(text = "1 + 2", number_linter()) +lint(text = "1 + 2", linters = number_linter()) } From 2b9e8604536f9ab2137ecf0512915d55d696668a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 07:36:28 +0000 Subject: [PATCH 6/7] pkgdown entry --- _pkgdown.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/_pkgdown.yaml b/_pkgdown.yaml index 290571031..8fd54393f 100644 --- a/_pkgdown.yaml +++ b/_pkgdown.yaml @@ -40,6 +40,7 @@ reference: - is_lint_level - get_r_string - use_lintr + - make_linter_from_xpath - xml_nodes_to_lints - xp_call_name From c19697823c460af26914a39635dc4ba71f2d8b88 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sat, 9 Sep 2023 07:38:47 +0000 Subject: [PATCH 7/7] robust to old stopifnot --- tests/testthat/test-make_linter_from_xpath.R | 28 ++++---------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/tests/testthat/test-make_linter_from_xpath.R b/tests/testthat/test-make_linter_from_xpath.R index 78a0aae5f..808d9be03 100644 --- a/tests/testthat/test-make_linter_from_xpath.R +++ b/tests/testthat/test-make_linter_from_xpath.R @@ -21,27 +21,9 @@ test_that("input validation works", { fixed = TRUE ) - expect_error( - make_linter_from_xpath(FALSE), - "xpath should be a character string", - fixed = TRUE - ) - - expect_error( - make_linter_from_xpath(letters), - "xpath should be a character string", - fixed = TRUE - ) - - expect_error( - make_linter_from_xpath(NA_character_), - "xpath should be a character string", - fixed = TRUE - ) - - expect_error( - make_linter_from_xpath(character()), - "xpath should be a character string", - fixed = TRUE - ) + err_msg <- if (getRversion() < "4.0.0") "is.character(xpath)" else "xpath should be a character string" + expect_error(make_linter_from_xpath(FALSE), err_msg, fixed = TRUE) + expect_error(make_linter_from_xpath(letters), err_msg, fixed = TRUE) + expect_error(make_linter_from_xpath(NA_character_), err_msg, fixed = TRUE) + expect_error(make_linter_from_xpath(character()), err_msg, fixed = TRUE) })