Improve message for seq_linter()

[IndrajeetPatil]

Closes #1471

TODO:

• Improve error message
• Adapt tests
• Refactor code (I am not sure if the nested ifelse() can be avoided)

library(lintr)

lint(
  text = "1:length(data.frame())",
  linters = seq_linter()
)
#> <text>:1:1: warning: [seq_linter] 1:length(...) is likely to be wrong in the empty edge case. Use seq_along(...) instead.
#> 1:length(data.frame())
#> ^~~~~~~~~~~~~~~~~~~~~~

lint(
  text = "seq(length(data.frame()))",
  linters = seq_linter()
)
#> <text>:1:1: warning: [seq_linter] seq(length(...)) is likely to be wrong in the empty edge case. Use seq_along(...) instead.
#> seq(length(data.frame()))
#> ^~~~~~~~~~~~~~~~~~~~~~~~~

lint(
  text = "1:nrow(data.frame())",
  linters = seq_linter()
)
#> <text>:1:1: warning: [seq_linter] 1:nrow(...) is likely to be wrong in the empty edge case. Use seq_len(nrow(...)) instead.
#> 1:nrow(data.frame())
#> ^~~~~~~~~~~~~~~~~~~~

lint(
  text = "seq(nrow(data.frame()))",
  linters = seq_linter()
)
#> <text>:1:1: warning: [seq_linter] seq(nrow(...)) is likely to be wrong in the empty edge case. Use seq_len(nrow(...)) instead.
#> seq(nrow(data.frame()))
#> ^~~~~~~~~~~~~~~~~~~~~~~

lint(
  text ="c(1:length(x), 1:nrow(y))",
  linters = seq_linter()
)
#> <text>:1:3: warning: [seq_linter] 1:length(...) is likely to be wrong in the empty edge case. Use seq_along(...) instead.
#> c(1:length(x), 1:nrow(y))
#>   ^~~~~~~~~~~
#> <text>:1:16: warning: [seq_linter] 1:nrow(...) is likely to be wrong in the empty edge case. Use seq_len(nrow(...)) instead.
#> c(1:length(x), 1:nrow(y))
#>                ^~~~~~~~~

^{Created on 2022-07-26 by the reprex package (v2.0.1)}

[MichaelChirico]

i think we can avoid the nested ifelse by setting dot_expr2 = "..." for the seq_along() branch:

dot_expr2[replacement == "seq_along"] <- "..."
lint_message <- ifelse(
  grepl("seq", dot_expr1, fixed = TRUE),
  ...
)

[IndrajeetPatil]

Tried this, but doesn't seem to work. 🤕

[IndrajeetPatil]

Feel free to push into the PR if you can get it to work. Maybe I missed something.

[IndrajeetPatil]

Btw, this is the second time we have talked about avoiding nested ifelse().

I am wondering if we should introduce a new linter to cover such cases?
Providing a concrete alternative in lint message will be difficult, though.

[MichaelChirico]

I am wondering if we should introduce a new linter to cover such cases?

There's already lintr::nested_ifelse_linter(), it's just not a default linter

Providing a concrete alternative in lint message will be difficult, though.

There's no requirement to do so. The message can suggest alternatives but doesn't need to be concrete. That's one of the main differences vs. styler, in fact -- styler generally has to be a bit more conservative & shouldn't touch code unless there's a clear, specific improvement, whereas lintr can call out code that can be improved without doing the improvement.

[MichaelChirico]

oh i see, i had missed that dot_expr2 was used twice. so introduced dot_expr3 instead

[IndrajeetPatil]

There's already lintr::nested_ifelse_linter(), it's just not a default linter

Oh, missed that completely! Good to know.

introduced dot_expr3 instead

Thanks for simplifying. Much more readable now!

[IndrajeetPatil]

Okay, I went through the docs and the code, but I don't see any explanation for why this is not a default linter.
I think it should be.

E.g., in a PR like the current, we wouldn't even have to rely on its usage being caught by manual code review; the linter GHA itself would notify the contributor about the added lint.

[MichaelChirico]

Great stuff! Agree on the message format. Looks way better.