From 60869cefd0e0823a0de27452c4a166efdc7a4802 Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Fri, 10 May 2024 14:57:40 +0200 Subject: [PATCH 1/8] Update scalar_in_linter.R (#1) Make scalar_in_linter configurable to allow projects to define additional %in% style functions like %notin%. --- R/scalar_in_linter.R | 12 ++++++++---- inst/lintr/linters.csv | 2 +- man/configurable_linters.Rd | 1 + man/linters.Rd | 4 ++-- man/scalar_in_linter.Rd | 7 +++++-- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index 77ca70285..7dc1dcbe0 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -7,6 +7,8 @@ #' `scalar %in% vector` is OK, because the alternative (`any(vector == scalar)`) #' is more circuitous & potentially less clear. #' +#' @param add_in_operators Character vector of additional functions that behave like the %in% operator +#' #' @examples #' # will produce lints #' lint( @@ -28,14 +30,16 @@ #' @evalRd rd_tags("scalar_in_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -scalar_in_linter <- function() { +scalar_in_linter <- function(add_in_operators = "%chin%") { # TODO(#2085): Extend to include other cases where the RHS is clearly a scalar # NB: all of logical, integer, double, hex, complex are parsed as NUM_CONST - xpath <- " - //SPECIAL[text() = '%in%' or text() = '%chin%'] + special <- paste(paste0("text() = '", c("%in%", add_in_operators), "'"), collapse = " or ") + + xpath <- paste0(" + //SPECIAL[", special, "] /following-sibling::expr[NUM_CONST[not(starts-with(text(), 'NA'))] or STR_CONST] /parent::expr - " + ") Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 539cdbc98..95af98b61 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -90,7 +90,7 @@ repeat_linter,style readability return_linter,style configurable default routine_registration_linter,best_practices efficiency robustness sample_int_linter,efficiency readability robustness -scalar_in_linter,readability consistency best_practices efficiency +scalar_in_linter,readability consistency best_practices efficiency configurable semicolon_linter,style readability default configurable semicolon_terminator_linter,defunct seq_linter,robustness efficiency consistency best_practices default diff --git a/man/configurable_linters.Rd b/man/configurable_linters.Rd index cb1c17a54..1c72fffab 100644 --- a/man/configurable_linters.Rd +++ b/man/configurable_linters.Rd @@ -44,6 +44,7 @@ The following linters are tagged with 'configurable': \item{\code{\link{quotes_linter}}} \item{\code{\link{redundant_ifelse_linter}}} \item{\code{\link{return_linter}}} +\item{\code{\link{scalar_in_linter}}} \item{\code{\link{semicolon_linter}}} \item{\code{\link{string_boundary_linter}}} \item{\code{\link{todo_comment_linter}}} diff --git a/man/linters.Rd b/man/linters.Rd index d2ba40da1..394bd6126 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -19,7 +19,7 @@ The following tags exist: \itemize{ \item{\link[=best_practices_linters]{best_practices} (63 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (11 linters)} -\item{\link[=configurable_linters]{configurable} (43 linters)} +\item{\link[=configurable_linters]{configurable} (44 linters)} \item{\link[=consistency_linters]{consistency} (32 linters)} \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} @@ -123,7 +123,7 @@ The following linters exist: \item{\code{\link{return_linter}} (tags: configurable, default, style)} \item{\code{\link{routine_registration_linter}} (tags: best_practices, efficiency, robustness)} \item{\code{\link{sample_int_linter}} (tags: efficiency, readability, robustness)} -\item{\code{\link{scalar_in_linter}} (tags: best_practices, consistency, efficiency, readability)} +\item{\code{\link{scalar_in_linter}} (tags: best_practices, configurable, consistency, efficiency, readability)} \item{\code{\link{semicolon_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{seq_linter}} (tags: best_practices, consistency, default, efficiency, robustness)} \item{\code{\link{sort_linter}} (tags: best_practices, efficiency, readability)} diff --git a/man/scalar_in_linter.Rd b/man/scalar_in_linter.Rd index be94fd1a1..33bd7b1db 100644 --- a/man/scalar_in_linter.Rd +++ b/man/scalar_in_linter.Rd @@ -4,7 +4,10 @@ \alias{scalar_in_linter} \title{Block usage like x \%in\% "a"} \usage{ -scalar_in_linter() +scalar_in_linter(add_in_operators = "\%chin\%") +} +\arguments{ +\item{add_in_operators}{Character vector of additional functions that behave like the \%in\% operator} } \description{ \code{vector \%in\% set} is appropriate for matching a vector to a set, but if @@ -38,5 +41,5 @@ lint( \link{linters} for a complete list of linters available in lintr. } \section{Tags}{ -\link[=best_practices_linters]{best_practices}, \link[=consistency_linters]{consistency}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} +\link[=best_practices_linters]{best_practices}, \link[=configurable_linters]{configurable}, \link[=consistency_linters]{consistency}, \link[=efficiency_linters]{efficiency}, \link[=readability_linters]{readability} } From b38f100abe13031be6e3d02d7545e1cc36a2ef70 Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Sat, 11 May 2024 01:38:14 +0200 Subject: [PATCH 2/8] Incorporate review feedback (#2) * Incorporate review feedback - Use glue in xpath, - Add changes to NEWS - Change default for scalar_in_linter - Make lint msg more open ended if another %in% operator was linted - Update tests to match new bahaviour --- NEWS.md | 1 + R/scalar_in_linter.R | 27 ++++++++++++++------------ man/scalar_in_linter.Rd | 9 ++++----- tests/testthat/test-scalar_in_linter.R | 10 +++++----- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/NEWS.md b/NEWS.md index dc089c28f..e41190182 100644 --- a/NEWS.md +++ b/NEWS.md @@ -54,6 +54,7 @@ * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico). * `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico). +* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. `%chin%` is no longer linted by default. (@F-Noelle) ### New linters diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index 7dc1dcbe0..add7fca01 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -1,13 +1,12 @@ #' Block usage like x %in% "a" #' #' `vector %in% set` is appropriate for matching a vector to a set, but if -#' that set has size 1, `==` is more appropriate. `%chin%` from `{data.table}` -#' is matched as well. +#' that set has size 1, `==` is more appropriate. #' #' `scalar %in% vector` is OK, because the alternative (`any(vector == scalar)`) #' is more circuitous & potentially less clear. #' -#' @param add_in_operators Character vector of additional functions that behave like the %in% operator +#' @param in_operators Character vector of additional functions that behave like the `%in%` operator #' #' @examples #' # will produce lints @@ -18,7 +17,7 @@ #' #' lint( #' text = "x %chin% 'a'", -#' linters = scalar_in_linter() +#' linters = scalar_in_linter(in_operators = "%chin%") #' ) #' #' # okay @@ -30,13 +29,11 @@ #' @evalRd rd_tags("scalar_in_linter") #' @seealso [linters] for a complete list of linters available in lintr. #' @export -scalar_in_linter <- function(add_in_operators = "%chin%") { +scalar_in_linter <- function(in_operators = NULL) { # TODO(#2085): Extend to include other cases where the RHS is clearly a scalar # NB: all of logical, integer, double, hex, complex are parsed as NUM_CONST - special <- paste(paste0("text() = '", c("%in%", add_in_operators), "'"), collapse = " or ") - - xpath <- paste0(" - //SPECIAL[", special, "] + xpath <- glue(" + //SPECIAL[{xp_text_in_table(c('%in%', {in_operators}))}] /following-sibling::expr[NUM_CONST[not(starts-with(text(), 'NA'))] or STR_CONST] /parent::expr ") @@ -46,13 +43,19 @@ scalar_in_linter <- function(add_in_operators = "%chin%") { bad_expr <- xml_find_all(xml, xpath) in_op <- xml_find_chr(bad_expr, "string(SPECIAL)") - lint_msg <- - paste0("Use == to match length-1 scalars, not ", in_op, ". Note that == preserves NA where ", in_op, " does not.") + msg <- ifelse( + in_op == "%in%", + "Use == to match length-1 scalars instead of %in%. Note that == preserves NA where %in% does not.", + glue( + "{in_op} behaves similar to %in%. ", + "Use operators like == to match length-1 scalars instead of operators like %in%." + ) + ) xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = lint_msg, + lint_message = msg, type = "warning" ) }) diff --git a/man/scalar_in_linter.Rd b/man/scalar_in_linter.Rd index 33bd7b1db..7372b08cd 100644 --- a/man/scalar_in_linter.Rd +++ b/man/scalar_in_linter.Rd @@ -4,15 +4,14 @@ \alias{scalar_in_linter} \title{Block usage like x \%in\% "a"} \usage{ -scalar_in_linter(add_in_operators = "\%chin\%") +scalar_in_linter(in_operators = NULL) } \arguments{ -\item{add_in_operators}{Character vector of additional functions that behave like the \%in\% operator} +\item{in_operators}{Character vector of additional functions that behave like the \code{\%in\%} operator} } \description{ \code{vector \%in\% set} is appropriate for matching a vector to a set, but if -that set has size 1, \code{==} is more appropriate. \verb{\%chin\%} from \code{{data.table}} -is matched as well. +that set has size 1, \code{==} is more appropriate. } \details{ \code{scalar \%in\% vector} is OK, because the alternative (\code{any(vector == scalar)}) @@ -27,7 +26,7 @@ lint( lint( text = "x \%chin\% 'a'", - linters = scalar_in_linter() + linters = scalar_in_linter(in_operators = "\%chin\%") ) # okay diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index 2bfd66f83..9e123ef3f 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -3,7 +3,7 @@ test_that("scalar_in_linter skips allowed usages", { expect_lint("x %in% y", NULL, linter) expect_lint("y %in% c('a', 'b')", NULL, linter) - expect_lint("c('a', 'b') %chin% x", NULL, linter) + expect_lint("c('a', 'b') %in% x", NULL, linter) expect_lint("z %in% 1:3", NULL, linter) # scalars on LHS are fine (often used as `"col" %in% names(DF)`) expect_lint("3L %in% x", NULL, linter) @@ -15,16 +15,16 @@ test_that("scalar_in_linter skips allowed usages", { }) test_that("scalar_in_linter blocks simple disallowed usages", { - linter <- scalar_in_linter() - lint_in_msg <- rex::rex("Use == to match length-1 scalars, not %in%.") - lint_chin_msg <- rex::rex("Use == to match length-1 scalars, not %chin%.") + linter <- scalar_in_linter(in_operators = "%chin%") + lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") + lint_chin_msg <- rex::rex("%chin% behaves similar to %in%.") expect_lint("x %in% 1", lint_in_msg, linter) expect_lint("x %chin% 'a'", lint_chin_msg, linter) }) test_that("multiple lints are generated correctly", { - linter <- scalar_in_linter() + linter <- scalar_in_linter(in_operators = "%chin%") expect_lint( trim_some('{ From 39a9c02f1fd44fc709636312c9ec2077d6a56297 Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Sat, 11 May 2024 11:36:40 +0200 Subject: [PATCH 3/8] Add a vector of in operators (#3) --- tests/testthat/test-scalar_in_linter.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index 9e123ef3f..7be38f123 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -15,12 +15,14 @@ test_that("scalar_in_linter skips allowed usages", { }) test_that("scalar_in_linter blocks simple disallowed usages", { - linter <- scalar_in_linter(in_operators = "%chin%") + linter <- scalar_in_linter(in_operators = c("%chin%", "%notin%")) lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") lint_chin_msg <- rex::rex("%chin% behaves similar to %in%.") + lint_notin_msg <- rex::rex("%notin% behaves similar to %in%.") expect_lint("x %in% 1", lint_in_msg, linter) expect_lint("x %chin% 'a'", lint_chin_msg, linter) + expect_lint("x %notin% 1", lint_notin_msg, linter) }) test_that("multiple lints are generated correctly", { From 7bd83243acc893510a8bdbca13bf5e81868f8c4f Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Tue, 14 May 2024 20:20:43 +0200 Subject: [PATCH 4/8] Add a testcase based on configuration (#4) --- tests/testthat/test-scalar_in_linter.R | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index 7be38f123..979478f6b 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -7,7 +7,6 @@ test_that("scalar_in_linter skips allowed usages", { expect_lint("z %in% 1:3", NULL, linter) # scalars on LHS are fine (often used as `"col" %in% names(DF)`) expect_lint("3L %in% x", NULL, linter) - # this should be is.na(x), but it more directly uses the "always TRUE/FALSE, _not_ NA" # aspect of %in%, so we delegate linting here to equals_na_linter() expect_lint("x %in% NA", NULL, linter) @@ -16,6 +15,7 @@ test_that("scalar_in_linter skips allowed usages", { test_that("scalar_in_linter blocks simple disallowed usages", { linter <- scalar_in_linter(in_operators = c("%chin%", "%notin%")) + lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") lint_chin_msg <- rex::rex("%chin% behaves similar to %in%.") lint_notin_msg <- rex::rex("%notin% behaves similar to %in%.") @@ -25,6 +25,24 @@ test_that("scalar_in_linter blocks simple disallowed usages", { expect_lint("x %notin% 1", lint_notin_msg, linter) }) +test_that("scalar_in_linter blocks or skips based on configuration", { + linter_default <- scalar_in_linter() + linter_config <- scalar_in_linter(in_operators = "%notin%") + + lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") + lint_notin_msg <- rex::rex("%notin% behaves similar to %in%.") + + # default + expect_lint("x %in% 1", lint_in_msg, linter_default) + expect_lint("x %notin% 1", NULL, linter_default) + expect_lint("x %notin% y", NULL, linter_default) + + # configured + expect_lint("x %in% 1", lint_in_msg, linter_config) + expect_lint("x %notin% 1", lint_notin_msg, linter_config) + expect_lint("x %notin% y", NULL, linter_config) +}) + test_that("multiple lints are generated correctly", { linter <- scalar_in_linter(in_operators = "%chin%") From 3f5eb7c5d4bce5a34d093acc966de9a0c4657112 Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Tue, 21 May 2024 02:55:50 +0200 Subject: [PATCH 5/8] Improve lint message, NEWS, param documentation (#5) --- NEWS.md | 2 +- R/scalar_in_linter.R | 14 +++++--------- man/scalar_in_linter.Rd | 2 +- tests/testthat/test-scalar_in_linter.R | 20 ++++++++------------ 4 files changed, 15 insertions(+), 23 deletions(-) diff --git a/NEWS.md b/NEWS.md index e41190182..73d8f27a9 100644 --- a/NEWS.md +++ b/NEWS.md @@ -54,7 +54,7 @@ * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico). * `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico). -* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. `%chin%` is no longer linted by default. (@F-Noelle) +* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle) ### New linters diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index add7fca01..f10b7ce09 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -6,7 +6,7 @@ #' `scalar %in% vector` is OK, because the alternative (`any(vector == scalar)`) #' is more circuitous & potentially less clear. #' -#' @param in_operators Character vector of additional functions that behave like the `%in%` operator +#' @param in_operators Character vector of additional infix operators that behave like the `%in%` operator #' #' @examples #' # will produce lints @@ -43,19 +43,15 @@ scalar_in_linter <- function(in_operators = NULL) { bad_expr <- xml_find_all(xml, xpath) in_op <- xml_find_chr(bad_expr, "string(SPECIAL)") - msg <- ifelse( - in_op == "%in%", - "Use == to match length-1 scalars instead of %in%. Note that == preserves NA where %in% does not.", - glue( - "{in_op} behaves similar to %in%. ", - "Use operators like == to match length-1 scalars instead of operators like %in%." - ) + lint_msg <- glue( + "Use comparison operators (e.g. ==) to match length-1 scalars instead of {in_op}. ", + "Note that == preserves NA where %in% does not." ) xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = msg, + lint_message = lint_msg, type = "warning" ) }) diff --git a/man/scalar_in_linter.Rd b/man/scalar_in_linter.Rd index 7372b08cd..4987a6a1d 100644 --- a/man/scalar_in_linter.Rd +++ b/man/scalar_in_linter.Rd @@ -7,7 +7,7 @@ scalar_in_linter(in_operators = NULL) } \arguments{ -\item{in_operators}{Character vector of additional functions that behave like the \code{\%in\%} operator} +\item{in_operators}{Character vector of additional infix operators that behave like the \code{\%in\%} operator} } \description{ \code{vector \%in\% set} is appropriate for matching a vector to a set, but if diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index 979478f6b..2cc4fdc55 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -15,31 +15,27 @@ test_that("scalar_in_linter skips allowed usages", { test_that("scalar_in_linter blocks simple disallowed usages", { linter <- scalar_in_linter(in_operators = c("%chin%", "%notin%")) + lint_msg <- rex::rex("Use comparison operators (e.g. ==) to match length-1 scalars instead of") - lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") - lint_chin_msg <- rex::rex("%chin% behaves similar to %in%.") - lint_notin_msg <- rex::rex("%notin% behaves similar to %in%.") - - expect_lint("x %in% 1", lint_in_msg, linter) - expect_lint("x %chin% 'a'", lint_chin_msg, linter) - expect_lint("x %notin% 1", lint_notin_msg, linter) + expect_lint("x %in% 1", lint_msg, linter) + expect_lint("x %chin% 'a'", lint_msg, linter) + expect_lint("x %notin% 1", lint_msg, linter) }) test_that("scalar_in_linter blocks or skips based on configuration", { linter_default <- scalar_in_linter() linter_config <- scalar_in_linter(in_operators = "%notin%") - lint_in_msg <- rex::rex("Use == to match length-1 scalars instead of %in%.") - lint_notin_msg <- rex::rex("%notin% behaves similar to %in%.") + lint_msg <- rex::rex("Use comparison operators (e.g. ==) to match length-1 scalars instead of") # default - expect_lint("x %in% 1", lint_in_msg, linter_default) + expect_lint("x %in% 1", lint_msg, linter_default) expect_lint("x %notin% 1", NULL, linter_default) expect_lint("x %notin% y", NULL, linter_default) # configured - expect_lint("x %in% 1", lint_in_msg, linter_config) - expect_lint("x %notin% 1", lint_notin_msg, linter_config) + expect_lint("x %in% 1", lint_msg, linter_config) + expect_lint("x %notin% 1", lint_msg, linter_config) expect_lint("x %notin% y", NULL, linter_config) }) From 04881ef75e35eac44313ad574446c4e725577afd Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Tue, 21 May 2024 03:16:07 +0200 Subject: [PATCH 6/8] Improve lint message (#6) --- R/scalar_in_linter.R | 4 ++-- tests/testthat/test-scalar_in_linter.R | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index f10b7ce09..0575bc51d 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -44,8 +44,8 @@ scalar_in_linter <- function(in_operators = NULL) { bad_expr <- xml_find_all(xml, xpath) in_op <- xml_find_chr(bad_expr, "string(SPECIAL)") lint_msg <- glue( - "Use comparison operators (e.g. ==) to match length-1 scalars instead of {in_op}. ", - "Note that == preserves NA where %in% does not." + "Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of {in_op} ", + "as these operators preserve NA where {in_op} does not." ) xml_nodes_to_lints( diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index 2cc4fdc55..fb3663087 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -15,7 +15,7 @@ test_that("scalar_in_linter skips allowed usages", { test_that("scalar_in_linter blocks simple disallowed usages", { linter <- scalar_in_linter(in_operators = c("%chin%", "%notin%")) - lint_msg <- rex::rex("Use comparison operators (e.g. ==) to match length-1 scalars instead of") + lint_msg <- rex::rex("Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of") expect_lint("x %in% 1", lint_msg, linter) expect_lint("x %chin% 'a'", lint_msg, linter) @@ -26,7 +26,7 @@ test_that("scalar_in_linter blocks or skips based on configuration", { linter_default <- scalar_in_linter() linter_config <- scalar_in_linter(in_operators = "%notin%") - lint_msg <- rex::rex("Use comparison operators (e.g. ==) to match length-1 scalars instead of") + lint_msg <- rex::rex("Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of") # default expect_lint("x %in% 1", lint_msg, linter_default) From 49c453e682460033d0ab2de76134cd3999730d86 Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Tue, 21 May 2024 08:43:23 +0200 Subject: [PATCH 7/8] Update R/scalar_in_linter.R Co-authored-by: Michael Chirico --- R/scalar_in_linter.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index 0575bc51d..f03de0fcd 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -6,7 +6,8 @@ #' `scalar %in% vector` is OK, because the alternative (`any(vector == scalar)`) #' is more circuitous & potentially less clear. #' -#' @param in_operators Character vector of additional infix operators that behave like the `%in%` operator +#' @param in_operators Character vector of additional infix operators that behave like the `%in%` operator, +#' e.g. `{data.table}`'s `%chin%` operator. #' #' @examples #' # will produce lints From 1aa6491cff44040e44f2e8e3495ca53c7dd48cfa Mon Sep 17 00:00:00 2001 From: F-Noelle Date: Tue, 21 May 2024 09:00:10 +0200 Subject: [PATCH 8/8] Update lint message (#7) --- NEWS.md | 2 +- R/scalar_in_linter.R | 4 ++-- man/scalar_in_linter.Rd | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/NEWS.md b/NEWS.md index 73d8f27a9..333fc6396 100644 --- a/NEWS.md +++ b/NEWS.md @@ -16,6 +16,7 @@ * `unnecessary_nested_if_linter()` is deprecated and subsumed into the new/more general `unnecessary_nesting_linter()`. * Drop support for posting GitHub comments from inside GitHub comment bot, Travis, Wercker, and Jenkins CI tools (spurred by #2148, @MichaelChirico). We rely on GitHub Actions for linting in CI, and don't see any active users relying on these alternatives. We welcome and encourage community contributions to get support for different CI system going again. * `cyclocomp_linter()` is no longer part of the default linters (#2555, @IndrajeetPatil) because the tidyverse style guide doesn't contain any guidelines on meeting certain complexity requirements. Note that users with `cyclocomp_linter()` in their configs may now need to install {cyclocomp} intentionally, in particular in CI/CD pipelines. +* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle) ## Bug fixes @@ -54,7 +55,6 @@ * `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico). * `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico). * `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico). -* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle) ### New linters diff --git a/R/scalar_in_linter.R b/R/scalar_in_linter.R index f03de0fcd..e6c9faace 100644 --- a/R/scalar_in_linter.R +++ b/R/scalar_in_linter.R @@ -45,8 +45,8 @@ scalar_in_linter <- function(in_operators = NULL) { bad_expr <- xml_find_all(xml, xpath) in_op <- xml_find_chr(bad_expr, "string(SPECIAL)") lint_msg <- glue( - "Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of {in_op} ", - "as these operators preserve NA where {in_op} does not." + "Use comparison operators (e.g. ==, !=, etc.) to match length-1 scalars instead of {in_op}. ", + "Note that comparison operators preserve NA where {in_op} does not." ) xml_nodes_to_lints( diff --git a/man/scalar_in_linter.Rd b/man/scalar_in_linter.Rd index 4987a6a1d..1773c699f 100644 --- a/man/scalar_in_linter.Rd +++ b/man/scalar_in_linter.Rd @@ -7,7 +7,8 @@ scalar_in_linter(in_operators = NULL) } \arguments{ -\item{in_operators}{Character vector of additional infix operators that behave like the \code{\%in\%} operator} +\item{in_operators}{Character vector of additional infix operators that behave like the \code{\%in\%} operator, +e.g. \code{{data.table}}'s \verb{\%chin\%} operator.} } \description{ \code{vector \%in\% set} is appropriate for matching a vector to a set, but if