Merge branch 'main' into if_not_else

Author: AshesITR

NEWS.md

@@ -30,6 +30,7 @@
 * `conjunct_test_linter()` also lints usage like `dplyr::filter(x, A & B)` in favor of using `dplyr::filter(x, A, B)` (part of #884, @MichaelChirico).
 * `sort_linter()` checks for code like `x == sort(x)` which is better served by using the function `is.unsorted()` (part of #884, @MichaelChirico).
 * `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.
 
 ### New linters

R/seq_linter.R

@@ -95,23 +95,24 @@ seq_linter <- function() {
 
     badx <- xml_find_all(xml, xpath)
 
-    # TODO: better message customization. For example, length(x):1
-    #   would get rev(seq_along(x)) as the preferred replacement.
     dot_expr1 <- get_fun(badx, 1L)
     dot_expr2 <- get_fun(badx, 2L)
     seq_along_idx <- grepl("length(", dot_expr1, fixed = TRUE) | grepl("length(", dot_expr2, fixed = TRUE)
-    replacement <- ifelse(seq_along_idx, "seq_along", "seq_len")
+    rev_idx <- startsWith(dot_expr2, "1")
+
+    replacement <- rep("seq_along(...)", length(badx))
+    replacement[!seq_along_idx] <- paste0("seq_len(", ifelse(rev_idx, dot_expr1, dot_expr2)[!seq_along_idx], ")")
+    replacement[rev_idx] <- paste0("rev(", replacement[rev_idx], ")")
 
-    dot_expr3 <- ifelse(seq_along_idx, "...", dot_expr2)
     lint_message <- ifelse(
       grepl("seq", dot_expr1, fixed = TRUE),
       sprintf(
-        "%s(%s) is likely to be wrong in the empty edge case. Use %s(%s) instead.",
-        dot_expr1, dot_expr2, replacement, dot_expr3
+        "%s(%s) is likely to be wrong in the empty edge case. Use %s instead.",
+        dot_expr1, dot_expr2, replacement
       ),
       sprintf(
-        "%s:%s is likely to be wrong in the empty edge case. Use %s(%s) instead.",
-        dot_expr1, dot_expr2, replacement, dot_expr3
+        "%s:%s is likely to be wrong in the empty edge case. Use %s instead.",
+        dot_expr1, dot_expr2, replacement
       )
     )
 

tests/testthat/test-seq_linter.R

@@ -116,7 +116,7 @@ test_that("reverse seq is ok", {
 
   expect_lint(
     "function(x) { length(x):1 }",
-    rex::rex("length(...):1", anything, "Use seq_along"),
+    rex::rex("length(...):1", anything, "Use rev(seq_along(...))"),
     seq_linter()
   )
 })
@@ -158,3 +158,12 @@ test_that("Message vectorization works for multiple lints", {
     seq_linter()
   )
 })
+
+test_that("Message recommends rev() correctly", {
+  linter <- seq_linter()
+
+  expect_lint(".N:1", rex::rex("Use rev(seq_len(.N))"), linter)
+  expect_lint("n():1", rex::rex("Use rev(seq_len(n()))"), linter)
+  expect_lint("nrow(x):1", rex::rex("Use rev(seq_len(nrow(...)))"), linter)
+  expect_lint("length(x):1", rex::rex("Use rev(seq_along(...))"), linter)
+})