Improve message for seq_linter() (#1475)

* Improve message for `seq_linter()`

Closes #1471

* update NEWS

* Update test-seq_linter.R

* simplify

Co-authored-by: Michael Chirico <[email protected]>
Co-authored-by: Michael Chirico <[email protected]>

Author: IndrajeetPatil

NEWS.md

@@ -19,6 +19,9 @@
 * `redundant_ifelse_linter()`'s lint message correctly suggests negation when 
   the `yes` condition is `0` (#1432, @IndrajeetPatil).
 
+* `seq_linter()` provides more specific replacement code in lint message 
+  (#1475, @IndrajeetPatil).
+
 ## New and improved features
 
 * `unreachable_code_linter()` ignores trailing comments if they match a closing nolint block (#1347, @AshesITR).

R/seq_linter.R

@@ -68,21 +68,19 @@ seq_linter <- function() {
     #   would get rev(seq_along(x)) as the preferred replacement.
     dot_expr1 <- get_fun(badx, 1L)
     dot_expr2 <- get_fun(badx, 2L)
-    replacement <- ifelse(
-      grepl("length(", dot_expr1, fixed = TRUE) | grepl("length(", dot_expr2, fixed = TRUE),
-      "seq_along",
-      "seq_len"
-    )
+    seq_along_idx <- grepl("length(", dot_expr1, fixed = TRUE) | grepl("length(", dot_expr2, fixed = TRUE)
+    replacement <- ifelse(seq_along_idx, "seq_along", "seq_len")
 
+    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(...) 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
       ),
       sprintf(
-        "%s:%s is likely to be wrong in the empty edge case. Use %s() instead.",
-        dot_expr1, dot_expr2, replacement
+        "%s:%s is likely to be wrong in the empty edge case. Use %s(%s) instead.",
+        dot_expr1, dot_expr2, replacement, dot_expr3
       )
     )
 

tests/testthat/test-seq_linter.R

@@ -26,7 +26,19 @@ test_that("finds seq(...) expressions", {
 
   expect_lint(
     "function(x) { seq(nrow(x)) }",
-    rex("seq(nrow(...))", anything, "Use seq_len(...)"),
+    rex("seq(nrow(...))", anything, "Use seq_len(nrow(...))"),
+    linter
+  )
+
+  expect_lint(
+    "function(x) { rev(seq(length(x))) }",
+    rex("seq(length(...))", anything, "Use seq_along(...)"),
+    linter
+  )
+
+  expect_lint(
+    "function(x) { rev(seq(nrow(x))) }",
+    rex("seq(nrow(...))", anything, "Use seq_len(nrow(...))"),
     linter
   )
 })
@@ -99,6 +111,10 @@ test_that("1L is also bad", {
 })
 
 test_that("reverse seq is ok", {
+  linter <- seq_linter()
+  expect_lint("function(x) { rev(seq_along(x)) }", NULL, linter)
+  expect_lint("function(x) { rev(seq_len(nrow(x))) }", NULL, linter)
+
   expect_lint(
     "function(x) { length(x):1 }",
     rex("length(...):1", anything, "Use seq_along"),
@@ -110,8 +126,8 @@ test_that("Message vectorization works for multiple lints", {
   expect_lint(
     "c(1:length(x), 1:nrow(y))",
     list(
-      rex::rex("1:length(...)", anything, "seq_along()"),
-      rex::rex("1:nrow(...)", anything, "seq_len()")
+      rex::rex("1:length(...)", anything, "seq_along(...)"),
+      rex::rex("1:nrow(...)", anything, "seq_len(nrow(...))")
     ),
     seq_linter()
   )
@@ -120,7 +136,7 @@ test_that("Message vectorization works for multiple lints", {
     "c(seq(length(x)), 1:nrow(y))",
     list(
       rex::rex("seq(length(...))", anything, "seq_along(...)"),
-      rex::rex("1:nrow(...)", anything, "seq_len()")
+      rex::rex("1:nrow(...)", anything, "seq_len(nrow(...))")
     ),
     seq_linter()
   )
@@ -129,7 +145,16 @@ test_that("Message vectorization works for multiple lints", {
     "c(seq(length(x)), seq(nrow(y)))",
     list(
       rex::rex("seq(length(...))", anything, "seq_along(...)"),
-      rex::rex("seq(nrow(...))", anything, "seq_len(...)")
+      rex::rex("seq(nrow(...))", anything, "seq_len(nrow(...))")
+    ),
+    seq_linter()
+  )
+
+  expect_lint(
+    "c(1:NROW(x), seq(NCOL(y)))",
+    list(
+      rex::rex("1:NROW(...)", anything, "seq_len(NROW(...)"),
+      rex::rex("seq(NCOL(...))", anything, "seq_len(NCOL(...))")
     ),
     seq_linter()
   )