diff --git a/NEWS.md b/NEWS.md index 688c18fab..cf11387b7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,7 @@ * `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) * `lint()` and friends now normalize paths to forward slashes on Windows (@olivroy, #2613). * `undesirable_function_linter()`, `undesirable_operator_linter()`, and `list_comparison_linter()` were removed from the tag `efficiency` (@IndrajeetPatil, #2655). If you use `linters_with_tags("efficiency")` to include these linters, you'll need to adjust your config to keep linting your code against them. We did not find any such users on GitHub. +* Arguments `allow_cascading_assign=`, `allow_right_assign=`, and `allow_pipe_assign=` to `assignment_linter()` are all deprecated in favor of the new `operator=` argument. Usage of a positional first argument like `assignment_linter(TRUE)`, of which we found 0 cases on GitHub, is totally deprecated to allow `operator=` to be positionally first. See below about the new argument. ## Bug fixes @@ -61,6 +62,7 @@ * `expect_no_lint()` was added as new function to cover the typical use case of expecting no lint message, akin to the recent {testthat} functions like `expect_no_warning()` (#2580, @F-Noelle). * `lint()` and friends emit a message if no lints are found (#2643, @IndrajeetPatil). * `{lintr}` now has a hex sticker (https://github.com/rstudio/hex-stickers/pull/110). Thank you, @gregswinehart! +* `assignment_linter()` can be fully customized with the new `operator=` argument to specify an exact vector of assignment operators to allow (#2441, @MichaelChirico and @J-Moravec). The default is `<-` and `<<-`; authors wishing to use `=` (only) for assignment in their codebase can use `operator = "="`. This supersedes several old arguments: to accomplish `allow_cascading_assign=TRUE`, add `"<<-"` (and/or `"->>"`) to `operator=`; for `allow_right_assign=TRUE`, add `"->"` (and/or `"->>"`) to `operator=`; for `allow_pipe_assign=TRUE`, add `"%<>%"` to `operator=`. Use `operator = "any"` to denote "ignore all assignment operators"; in this case, only the value of `allow_trailing=` matters. Implicit assignments with `<-` are always ignored by `assignment_linter()`; use `implicit_assignment_linter()` to handle linting these. ### New linters diff --git a/R/assignment_linter.R b/R/assignment_linter.R index da42b5119..cd146f2ed 100644 --- a/R/assignment_linter.R +++ b/R/assignment_linter.R @@ -1,12 +1,15 @@ #' Assignment linter #' -#' Check that `<-` is always used for assignment. +#' Check that the specified operator is used for assignment. #' -#' @param allow_cascading_assign Logical, default `TRUE`. +#' @param operator Character vector of valid assignment operators. Defaults to allowing `<-` and `<<-`; other valid +#' options are `=`, `->`, `->>`, `%<>%`; use `"any"` to denote "allow all operators", in which case this linter only +#' considers `allow_trailing` for generating lints. +#' @param allow_cascading_assign (Deprecated) Logical, default `TRUE`. #' If `FALSE`, [`<<-`][base::assignOps] and `->>` are not allowed. -#' @param allow_right_assign Logical, default `FALSE`. If `TRUE`, `->` and `->>` are allowed. +#' @param allow_right_assign (Deprecated) Logical, default `FALSE`. If `TRUE`, `->` and `->>` are allowed. #' @param allow_trailing Logical, default `TRUE`. If `FALSE` then assignments aren't allowed at end of lines. -#' @param allow_pipe_assign Logical, default `FALSE`. If `TRUE`, magrittr's `%<>%` assignment is allowed. +#' @param allow_pipe_assign (Deprecated) Logical, default `FALSE`. If `TRUE`, magrittr's `%<>%` assignment is allowed. #' #' @examples #' # will produce lints @@ -27,6 +30,11 @@ #' linters = assignment_linter() #' ) #' +#' lint( +#' text = "x <- 1", +#' linters = assignment_linter(operator = "=") +#' ) +#' #' # okay #' lint( #' text = "x <- mean(x)", @@ -64,63 +72,122 @@ #' linters = assignment_linter(allow_pipe_assign = TRUE) #' ) #' +#' lint( +#' text = "x = 1", +#' linters = assignment_linter(operator = "=") +#' ) +#' #' @evalRd rd_tags("assignment_linter") #' @seealso #' - [linters] for a complete list of linters available in lintr. #' - #' - #' @export -assignment_linter <- function(allow_cascading_assign = TRUE, +assignment_linter <- function(operator = c("<-", "<<-"), + allow_cascading_assign = TRUE, allow_right_assign = FALSE, allow_trailing = TRUE, allow_pipe_assign = FALSE) { - trailing_assign_xpath <- paste( + if (is.logical(operator)) { + cli_abort(c( + "'operator' should be a character vector but was logical.", + i = "If you intended positional usage of allow_cascading_assign=, that is hard-deprecated." + )) + } + if (!missing(allow_cascading_assign)) { + lintr_deprecated("allow_cascading_assign", '"<<-" and/or "->>" in operator', version = "3.2.0", type = "Argument") + operator <- drop_or_add(operator, "<<-", allow_cascading_assign) + } + if (!missing(allow_right_assign)) { + lintr_deprecated("allow_right_assign", '"->" in operator', version = "3.2.0", type = "Argument") + operator <- drop_or_add(operator, c("->", if (allow_cascading_assign) "->>"), allow_right_assign) + } + if (!missing(allow_pipe_assign)) { + lintr_deprecated("allow_pipe_assign", '"%<>%" in operator', version = "3.2.0", type = "Argument") + operator <- drop_or_add(operator, "%<>%", allow_pipe_assign) + } + all_operators <- c("<-", "=", "->", "<<-", "->>", "%<>%") + if ("any" %in% operator) { + operator <- all_operators + } else { + operator <- match.arg(operator, all_operators, several.ok = TRUE) + } + no_cascading <- !any(c("<<-", "->>") %in% operator) + trailing_assign_xpath <- paste0( collapse = " | ", c( - paste0("//LEFT_ASSIGN", if (allow_cascading_assign) "" else "[text() = '<-']"), - if (allow_right_assign) paste0("//RIGHT_ASSIGN", if (allow_cascading_assign) "" else "[text() = '->']"), + "//LEFT_ASSIGN", + "//RIGHT_ASSIGN", + "//EQ_ASSIGN", "//EQ_SUB", "//EQ_FORMALS", - if (!allow_pipe_assign) "//SPECIAL[text() = '%<>%']" + "//SPECIAL[text() = '%<>%']" ), "[@line1 < following-sibling::expr[1]/@line1]" ) - xpath <- paste(collapse = " | ", c( - # always block = (NB: the parser differentiates EQ_ASSIGN, EQ_SUB, and EQ_FORMALS) - "//EQ_ASSIGN", + op_xpath_parts <- c( + if (!"=" %in% operator) "//EQ_ASSIGN", # -> and ->> are both 'RIGHT_ASSIGN' - if (!allow_right_assign) "//RIGHT_ASSIGN" else if (!allow_cascading_assign) "//RIGHT_ASSIGN[text() = '->>']", + glue("//RIGHT_ASSIGN[{ xp_text_in_table(setdiff(c('->', '->>'), operator)) }]"), # <-, :=, and <<- are all 'LEFT_ASSIGN'; check the text if blocking <<-. # NB: := is not linted because of (1) its common usage in rlang/data.table and # (2) it's extremely uncommon as a normal assignment operator - if (!allow_cascading_assign) "//LEFT_ASSIGN[text() = '<<-']", - if (!allow_trailing) trailing_assign_xpath, - if (!allow_pipe_assign) "//SPECIAL[text() = '%<>%']" - )) + glue("//LEFT_ASSIGN[{ xp_text_in_table(setdiff(c('<-', '<<-'), operator)) }]"), + if (!"%<>%" %in% operator) "//SPECIAL[text() = '%<>%']" + ) + if (!is.null(op_xpath_parts)) { + # NB: Also used, essentially, in implicit_assignment_linter. Keep in sync. + implicit_assignment_xpath <- " + [not(parent::expr[ + preceding-sibling::*[2][self::IF or self::WHILE] + or parent::forcond + or parent::expr/*[1][self::OP-LEFT-PAREN] + ])] + " + op_xpath <- paste0(op_xpath_parts, implicit_assignment_xpath, collapse = " | ") + } else { + op_xpath <- NULL + } Linter(linter_level = "expression", function(source_expression) { xml <- source_expression$xml_parsed_content - bad_expr <- xml_find_all(xml, xpath) - if (length(bad_expr) == 0L) { - return(list()) - } + lints <- NULL + if (!is.null(op_xpath)) { + op_expr <- xml_find_all(xml, op_xpath) - operator <- xml_text(bad_expr) - lint_message_fmt <- rep("Use <-, not %s, for assignment.", length(operator)) - lint_message_fmt[operator %in% c("<<-", "->>")] <- - "Replace %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." - lint_message_fmt[operator == "%<>%"] <- - "Avoid the assignment pipe %s; prefer using <- and %%>%% separately." + op_text <- xml_text(op_expr) + op_lint_message_fmt <- rep( + sprintf( + "Use %s for assignment, not %%s.", + if (length(operator) > 1L) paste("one of", toString(operator)) else operator + ), + length(op_text) + ) + if (no_cascading) { + op_lint_message_fmt[op_text %in% c("<<-", "->>")] <- + "Replace %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." + } + op_lint_message_fmt[op_text == "%<>%"] <- + "Avoid the assignment pipe %s; prefer pipes and assignment in separate steps." + + op_lint_message <- sprintf(op_lint_message_fmt, op_text) + lints <- xml_nodes_to_lints(op_expr, source_expression, op_lint_message, type = "style") + } if (!allow_trailing) { - bad_trailing_expr <- xml_find_all(xml, trailing_assign_xpath) - trailing_assignments <- xml2::xml_attrs(bad_expr) %in% xml2::xml_attrs(bad_trailing_expr) - lint_message_fmt[trailing_assignments] <- "Assignment %s should not be trailing at the end of a line." + trailing_assign_expr <- xml_find_all(xml, trailing_assign_xpath) + trailing_assign_text <- xml_text(trailing_assign_expr) + trailing_assign_msg_fmt <- "Assignment %s should not be trailing at the end of a line." + trailing_assign_msg <- sprintf(trailing_assign_msg_fmt, trailing_assign_text) + lints <- c(lints, + xml_nodes_to_lints(trailing_assign_expr, source_expression, trailing_assign_msg, type = "style") + ) } - lint_message <- sprintf(lint_message_fmt, operator) - xml_nodes_to_lints(bad_expr, source_expression, lint_message, type = "style") + lints }) } + +drop_or_add <- function(x, y, add) (if (add) union else setdiff)(x, y) diff --git a/R/implicit_assignment_linter.R b/R/implicit_assignment_linter.R index 70dfd3376..758c8ab6e 100644 --- a/R/implicit_assignment_linter.R +++ b/R/implicit_assignment_linter.R @@ -79,6 +79,7 @@ implicit_assignment_linter <- function(except = c("bquote", "expression", "expr" sep = " | " ) + # NB: Also used, essentially, in assignment_linter. Keep in sync. xpath <- glue(" ({assignments}) /parent::expr[ diff --git a/man/assignment_linter.Rd b/man/assignment_linter.Rd index 291343fb2..9efc4bbd1 100644 --- a/man/assignment_linter.Rd +++ b/man/assignment_linter.Rd @@ -5,6 +5,7 @@ \title{Assignment linter} \usage{ assignment_linter( + operator = c("<-", "<<-"), allow_cascading_assign = TRUE, allow_right_assign = FALSE, allow_trailing = TRUE, @@ -12,17 +13,21 @@ assignment_linter( ) } \arguments{ -\item{allow_cascading_assign}{Logical, default \code{TRUE}. +\item{operator}{Character vector of valid assignment operators. Defaults to allowing \verb{<-} and \verb{<<-}; other valid +options are \code{=}, \verb{->}, \verb{->>}, \verb{\%<>\%}; use \code{"any"} to denote "allow all operators", in which case this linter only +considers \code{allow_trailing} for generating lints.} + +\item{allow_cascading_assign}{(Deprecated) Logical, default \code{TRUE}. If \code{FALSE}, \code{\link[base:assignOps]{<<-}} and \verb{->>} are not allowed.} -\item{allow_right_assign}{Logical, default \code{FALSE}. If \code{TRUE}, \verb{->} and \verb{->>} are allowed.} +\item{allow_right_assign}{(Deprecated) Logical, default \code{FALSE}. If \code{TRUE}, \verb{->} and \verb{->>} are allowed.} \item{allow_trailing}{Logical, default \code{TRUE}. If \code{FALSE} then assignments aren't allowed at end of lines.} -\item{allow_pipe_assign}{Logical, default \code{FALSE}. If \code{TRUE}, magrittr's \verb{\%<>\%} assignment is allowed.} +\item{allow_pipe_assign}{(Deprecated) Logical, default \code{FALSE}. If \code{TRUE}, magrittr's \verb{\%<>\%} assignment is allowed.} } \description{ -Check that \verb{<-} is always used for assignment. +Check that the specified operator is used for assignment. } \examples{ # will produce lints @@ -43,6 +48,11 @@ lint( linters = assignment_linter() ) +lint( + text = "x <- 1", + linters = assignment_linter(operator = "=") +) + # okay lint( text = "x <- mean(x)", @@ -80,6 +90,11 @@ lint( linters = assignment_linter(allow_pipe_assign = TRUE) ) +lint( + text = "x = 1", + linters = assignment_linter(operator = "=") +) + } \seealso{ \itemize{ diff --git a/tests/testthat/test-assignment_linter.R b/tests/testthat/test-assignment_linter.R index bae8a048e..fb421dc20 100644 --- a/tests/testthat/test-assignment_linter.R +++ b/tests/testthat/test-assignment_linter.R @@ -9,7 +9,7 @@ test_that("assignment_linter skips allowed usages", { test_that("assignment_linter blocks disallowed usages", { linter <- assignment_linter() - lint_msg <- rex::rex("Use <-, not =, for assignment.") + lint_msg <- rex::rex("Use one of <-, <<- for assignment, not =.") expect_lint("blah=1", lint_msg, linter) expect_lint("blah = 1", lint_msg, linter) @@ -28,91 +28,140 @@ test_that("assignment_linter blocks disallowed usages", { test_that("arguments handle <<- and ->/->> correctly", { linter <- assignment_linter() + linter_yes_right <- assignment_linter(operator = c("->", "->>")) lint_msg_right <- rex::rex("Replace ->> by assigning to a specific environment") - expect_lint("1 -> blah", rex::rex("Use <-, not ->, for assignment."), linter) - expect_lint("1 ->> blah", lint_msg_right, linter) + expect_lint("1 -> blah", rex::rex("Use one of <-, <<- for assignment, not ->."), linter) + expect_lint("1 ->> blah", lint_msg_right, assignment_linter(operator = "<-")) # <<- is only blocked optionally expect_lint("1 <<- blah", NULL, linter) expect_lint( "1 <<- blah", rex::rex("Replace <<- by assigning to a specific environment"), - assignment_linter(allow_cascading_assign = FALSE) + assignment_linter(operator = "<-") ) # blocking -> can be disabled - expect_lint("1 -> blah", NULL, assignment_linter(allow_right_assign = TRUE)) - expect_lint("1 ->> blah", NULL, assignment_linter(allow_right_assign = TRUE)) - # blocked under cascading assign but not under right assign --> blocked + expect_lint("1 -> blah", NULL, linter_yes_right) + expect_lint("1 ->> blah", NULL, linter_yes_right) + # we can also differentiate '->' and '->>' expect_lint( "1 ->> blah", lint_msg_right, - assignment_linter(allow_cascading_assign = FALSE, allow_right_assign = TRUE) + assignment_linter(operator = c("<-", "->")) + ) + + # when user allows _some_ cascading assignment, advice should not mention the + # problems with cascading assignment, but focus on the specific disallowed operator. + expect_lint( + "1 ->> blah", + rex::rex("Use one of <-, <<- for assignment, not ->>."), + assignment_linter(operator = c("<-", "<<-")) + ) + expect_lint( + "blah <<- 1", + rex::rex("Use one of ->, ->> for assignment, not <<-."), + assignment_linter(operator = c("->", "->>")) ) }) test_that("arguments handle trailing assignment operators correctly", { - expect_lint("x <- y", NULL, assignment_linter(allow_trailing = FALSE)) - expect_lint("foo(bar = 1)", NULL, assignment_linter(allow_trailing = FALSE)) + linter_default <- assignment_linter() + linter_no_trailing <- assignment_linter(allow_trailing = FALSE) + expect_lint("x <- y", NULL, linter_no_trailing) + expect_lint("foo(bar = 1)", NULL, linter_no_trailing) expect_lint( - "foo(bar =\n1)", + trim_some(" + foo(bar = + 1) + "), rex::rex("= should not be trailing at the end of a line."), - assignment_linter(allow_trailing = FALSE) + linter_no_trailing ) expect_lint( - "x <<-\ny", + trim_some(" + x <<- + y + "), rex::rex("<<- should not be trailing"), - assignment_linter(allow_trailing = FALSE) + linter_no_trailing ) expect_lint( - "x <<-\ny", - rex::rex("Replace <<- by assigning to a specific environment"), - assignment_linter(allow_trailing = FALSE, allow_cascading_assign = FALSE) + trim_some(" + x <<- + y + "), + list( + rex::rex("Replace <<- by assigning to a specific environment"), + rex::rex("Assignment <<- should not be trailing") + ), + assignment_linter(operator = "<-", allow_trailing = FALSE) ) expect_lint( - "x <- #Test \ny", + trim_some(" + x <- #Test + y + "), rex::rex("<- should not be trailing"), - assignment_linter(allow_trailing = FALSE) + linter_no_trailing ) - expect_lint( - "is_long <-\nis %>%\ngather(measure, value, -Species) %>%\narrange(-value)", - NULL, - assignment_linter() - ) - expect_lint( - "is_long <-\nis %>%\ngather(measure, value, -Species) %>%\narrange(-value)", - rex::rex("<- should not be trailing"), - assignment_linter(allow_trailing = FALSE) - ) + pipe_left_string <- trim_some(" + is_long <- + is %>% + gather(measure, value, -Species) %>% + arrange(-value) + ") + expect_lint(pipe_left_string, NULL, linter_default) + expect_lint(pipe_left_string, rex::rex("<- should not be trailing"), linter_no_trailing) + pipe_right_string <- trim_some(" + is %>% + gather(measure, value, -Species) %>% + arrange(-value) -> + is_long + ") + expect_lint(pipe_right_string, rex::rex("Use one of <-, <<- for assignment, not ->"), linter_default) expect_lint( - "is %>%\ngather(measure, value, -Species) %>%\narrange(-value) ->\nis_long", - rex::rex("Use <-, not ->"), - assignment_linter() - ) - expect_lint( - "is %>%\ngather(measure, value, -Species) %>%\narrange(-value) ->\nis_long", - rex::rex("Use <-, not ->"), - assignment_linter(allow_trailing = FALSE) + pipe_right_string, + list( + rex::rex("Use one of <-, <<- for assignment, not ->"), + rex::rex("Assignment -> should not be trailing") + ), + linter_no_trailing ) expect_lint( - "is %>%\ngather(measure, value, -Species) %>%\narrange(-value) ->\nis_long", + pipe_right_string, rex::rex("-> should not be trailing"), - assignment_linter(allow_right_assign = TRUE, allow_trailing = FALSE) + assignment_linter(operator = "->", allow_trailing = FALSE) ) expect_lint( - "\n\nblah=\n42\nblh2<-\n54", + trim_some(" + blah = + 42 + blh2 <- + 54 + "), list( - list(message = "=", line_number = 3L, column_number = 5L), - list(message = "<-", line_number = 5L, column_number = 5L) + list(message = "Use one of <-, <<- for assignment, not =.", line_number = 1L, column_number = 6L), + list(message = "Assignment = should not be trailing at the end of a line", line_number = 1L, column_number = 6L), + list(message = "Assignment <- should not be trailing at the end of a line", line_number = 3L, column_number = 6L) ), - assignment_linter(allow_trailing = FALSE) + linter_no_trailing + ) + + expect_lint( + trim_some(" + a = + 1 + "), + "= should not be trailing", + assignment_linter(operator = "=", allow_trailing = FALSE) ) }) @@ -169,10 +218,20 @@ test_that("allow_trailing interacts correctly with comments in braced expression test_that("%<>% throws a lint", { expect_lint("x %<>% sum()", "Avoid the assignment pipe %<>%", assignment_linter()) - expect_lint("x %<>% sum()", NULL, assignment_linter(allow_pipe_assign = TRUE)) + expect_lint("x %<>% sum()", NULL, assignment_linter(operator = "%<>%")) # interaction with allow_trailing - expect_lint("x %<>%\n sum()", "Assignment %<>% should not be trailing", assignment_linter(allow_trailing = FALSE)) + expect_lint( + trim_some(" + x %<>% + sum() + "), + list( + "Avoid the assignment pipe %<>%", + "Assignment %<>% should not be trailing" + ), + assignment_linter(allow_trailing = FALSE) + ) }) test_that("multiple lints throw correct messages", { @@ -186,9 +245,204 @@ test_that("multiple lints throw correct messages", { list( list(message = "Replace <<- by assigning to a specific environment", line_number = 2L), list(message = "Replace ->> by assigning to a specific environment", line_number = 3L), - list(message = "Use <-, not ->", line_number = 4L), + list(message = "Use <- for assignment, not ->", line_number = 4L), + list(message = "Avoid the assignment pipe %<>%", line_number = 5L) + ), + assignment_linter(operator = "<-") + ) +}) + +test_that("assignment operator can be toggled", { + eq_linter <- assignment_linter(operator = "=") + any_linter <- assignment_linter(operator = "any") + lint_message <- rex("Use = for assignment, not") + + expect_lint("a = 1", NULL, eq_linter) + expect_lint("a = 1", NULL, any_linter) + + expect_lint("a <- 1", lint_message, eq_linter) + expect_lint("a <- 1", NULL, any_linter) + + expect_lint("a = 1; b <- 2", lint_message, eq_linter) + expect_lint("a = 1; b <- 2", NULL, any_linter) + + expect_lint( + trim_some(" + foo = function() { + a = 1 + } + "), + NULL, + eq_linter + ) + expect_lint( + trim_some(" + foo = function() { + a = 1 + } + "), + NULL, + any_linter + ) + + expect_lint( + trim_some(" + foo = function() { + a <- 1 + } + "), + list(lint_message, line_number = 2L), + eq_linter + ) + expect_lint( + trim_some(" + foo = function() { + a <- 1 + } + "), + NULL, + any_linter + ) + + expect_lint("if ({a = TRUE}) 1", NULL, eq_linter) + expect_lint("if ({a = TRUE}) 1", NULL, any_linter) + + expect_lint("if (a <- TRUE) 1", NULL, eq_linter) + expect_lint("if (a <- TRUE) 1", NULL, any_linter) + + expect_lint("for (ii in {a = TRUE}) 1", NULL, eq_linter) + expect_lint("for (ii in {a = TRUE}) 1", NULL, any_linter) + + expect_lint("for (ii in a <- TRUE) 1", NULL, eq_linter) + expect_lint("for (ii in a <- TRUE) 1", NULL, any_linter) + + expect_lint( + trim_some(" + x = + 2 + y <- + 3 + "), + list( + list("Assignment = should not be trailing", line_number = 1L), + list("Assignment <- should not be trailing", line_number = 3L) + ), + assignment_linter(operator = "any", allow_trailing = FALSE) + ) +}) + +test_that("multiple lints throw correct messages when both = and <- are allowed", { + expect_lint( + trim_some("{ + x <<- 1 + y ->> 2 + z -> 3 + x %<>% as.character() + foo <- 1 + bar = 2 + }"), + list( + list(message = "Replace <<- by assigning to a specific environment", line_number = 2L), + list(message = "Replace ->> by assigning to a specific environment", line_number = 3L), + list(message = "Use one of =, <- for assignment, not ->", line_number = 4L), + list(message = "Avoid the assignment pipe %<>%", line_number = 5L) + ), + assignment_linter(operator = c("=", "<-")) + ) +}) + +test_that("multiple lints throw correct messages when = is required", { + expect_lint( + trim_some("{ + x <<- 1 + y ->> 2 + z -> 3 + x %<>% as.character() + foo <- 1 + bar = 2 + }"), + list( + list(message = "Replace <<- by assigning to a specific environment", line_number = 2L), + list(message = "Replace ->> by assigning to a specific environment", line_number = 3L), + list(message = "Use = for assignment, not ->", line_number = 4L), + list(message = "Avoid the assignment pipe %<>%", line_number = 5L), + list(message = "Use = for assignment, not <-", line_number = 6L) + ), + assignment_linter(operator = "=") + ) +}) + +# tests copy-pasted from earlier suite and embellished with warnings; to be removed +test_that("Deprecated arguments work & warn as intended", { + expect_warning(regexp = "allow_cascading_assign", { + linter_no_cascade <- assignment_linter(allow_cascading_assign = FALSE) + }) + # Positionally-passed argument is hard-deprecated since we changed argument order. + expect_error(assignment_linter(FALSE), "allow_cascading_assign") + expect_warning(regexp = "allow_right_assign", { + linter_yes_right <- assignment_linter(allow_right_assign = TRUE) + }) + expect_warning(regexp = "allow_right_assign", expect_warning(regexp = "allow_cascading_assign", { + linter_no_cascade_yes_right <- assignment_linter(allow_cascading_assign = FALSE, allow_right_assign = TRUE) + })) + expect_warning(regexp = "allow_cascading_assign", { + linter_no_cascade_no_trailing <- assignment_linter(allow_trailing = FALSE, allow_cascading_assign = FALSE) + }) + expect_warning(regexp = "allow_right_assign", { + linter_yes_right_no_trailing <- assignment_linter(allow_right_assign = TRUE, allow_trailing = FALSE) + }) + expect_warning(regexp = "allow_pipe_assign", { + linter_yes_pipe <- assignment_linter(allow_pipe_assign = TRUE) + }) + + expect_lint( + "1 <<- blah", + rex::rex("Replace <<- by assigning to a specific environment"), + linter_no_cascade + ) + expect_lint("1 -> blah", NULL, linter_yes_right) + expect_lint("1 ->> blah", NULL, linter_yes_right) + + expect_lint( + "1 ->> blah", + rex::rex("Replace ->> by assigning to a specific environment"), + linter_no_cascade_yes_right + ) + expect_lint( + trim_some(" + x <<- + y + "), + list( + rex::rex("Replace <<- by assigning to a specific environment"), + rex::rex("Assignment <<- should not be trailing") + ), + linter_no_cascade_no_trailing + ) + expect_lint( + trim_some(" + is %>% + gather(measure, value, -Species) %>% + arrange(-value) -> + is_long + "), + rex::rex("-> should not be trailing"), + linter_yes_right_no_trailing + ) + expect_lint("x %<>% sum()", NULL, linter_yes_pipe) + expect_lint( + trim_some("{ + x <<- 1 + y ->> 2 + z -> 3 + x %<>% as.character() + }"), + list( + list(message = "Replace <<- by assigning to a specific environment", line_number = 2L), + list(message = "Replace ->> by assigning to a specific environment", line_number = 3L), + list(message = "Use <- for assignment, not ->", line_number = 4L), list(message = "Avoid the assignment pipe %<>%", line_number = 5L) ), - assignment_linter(allow_cascading_assign = FALSE) + linter_no_cascade ) }) diff --git a/tests/testthat/test-exclusions.R b/tests/testthat/test-exclusions.R index d4e8dd5c6..9fce8adf5 100644 --- a/tests/testthat/test-exclusions.R +++ b/tests/testthat/test-exclusions.R @@ -194,7 +194,7 @@ test_that("next-line exclusion works", { # NLN: line_length_linter. x = 1 "), - rex::rex("Use <-, not =, for assignment."), + rex::rex("Use one of <-, <<- for assignment, not =."), list(linter, line_length_linter()) ) @@ -204,7 +204,7 @@ test_that("next-line exclusion works", { x = 1 # NLN: assignment_linter. x = 2 "), - list(rex::rex("Use <-, not =, for assignment."), line_number = 1L), + list(rex::rex("Use one of <-, <<- for assignment, not =."), line_number = 1L), linter ) }) diff --git a/tests/testthat/test-expect_lint.R b/tests/testthat/test-expect_lint.R index f03d7e7a3..c2b9d6264 100644 --- a/tests/testthat/test-expect_lint.R +++ b/tests/testthat/test-expect_lint.R @@ -3,7 +3,7 @@ # for failure, always put the lint check or lint field that must fail first. linter <- assignment_linter() -lint_msg <- "Use <-, not =" +lint_msg <- "Use one of <-, <<- for assignment, not =" test_that("no checks", { expect_success(expect_no_lint("a", linter)) diff --git a/tests/testthat/test-knitr_extended_formats.R b/tests/testthat/test-knitr_extended_formats.R index 35dcbd652..15526265e 100644 --- a/tests/testthat/test-knitr_extended_formats.R +++ b/tests/testthat/test-knitr_extended_formats.R @@ -4,7 +4,7 @@ test_that("marginfigure engine from tufte package doesn't cause problems", { expect_lint( file = test_path("knitr_extended_formats", "tufte.Rmd"), - checks = list(rex::rex("Use <-, not =, for assignment."), line_number = 11L), + checks = list(rex::rex("Use one of <-, <<- for assignment, not =."), line_number = 11L), default_linters, parse_settings = FALSE ) @@ -16,7 +16,7 @@ test_that("engines from bookdown package cause no problems", { expect_lint( file = test_path("knitr_extended_formats", "bookdown.Rmd"), - checks = list(rex::rex("Use <-, not =, for assignment."), line_number = 14L), + checks = list(rex::rex("Use one of <-, <<- for assignment, not =."), line_number = 14L), default_linters, parse_settings = FALSE ) diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R index a62d23e2a..eb3dfc5f9 100644 --- a/tests/testthat/test-knitr_formats.R +++ b/tests/testthat/test-knitr_formats.R @@ -1,5 +1,5 @@ regexes <- list( - assign = rex::rex("Use <-, not =, for assignment."), + assign = rex::rex("Use one of <-, <<- for assignment, not =."), local_var = rex::rex("local variable"), quotes = rex::rex("Only use double-quotes."), trailing = rex::rex("Remove trailing blank lines."),