False positive in indentation_linter()?

[IndrajeetPatil]

Possibly related to r-lib/styler#1065.

library(lintr)

'test_that("bla", {
  x <-
    letters[1:3] %>%
    paste0() %>%
    length()
})' -> code

lint(
  text = code,
  linters = indentation_linter()
)
#> <text>:4:4: style: [indentation_linter] Indentation should be 6 spaces but is 4 spaces.
#>     paste0() %>%
#>    ^~~

^{Created on 2022-12-05 with reprex v2.0.2}

This is what {lintr} expects:

test_that("bla", {
  x <-
    letters[1:3] %>%
      paste0() %>%
      length()
})

But {styler} changes this to the above-mentioned version that produces lints.

[AshesITR]

The reason for this is that the indent triggered by <- reaches all the way to the end of the expression (i.e. upto length()) and the magrittr pipes (and base R pipes too) trigger indents for the following line due to left associativity:

"%t%" <- function(l, r) { list(l = l, r = r) }
1 %t% 2 %t% 3
#> $l
#> $l$l
#> [1] 1
#> 
#> $l$r
#> [1] 2
#> 
#> 
#> $r
#> [1] 3

^{Created on 2022-12-05 with reprex v2.0.2}

Seems like a matter of taste if this syntactical difference between %% and <- should be reflected in the indentation.

To me, a trailing <- looks very odd anyway. I would suggest using

x <- letters[1:3] %>%
  paste0() %>%
  length()

instead.

---

I just checked PyCharms Auto-Indent and it agrees with lintr.

[IndrajeetPatil]

Thanks for the explanation.

Makes sense to not have a trailing <-, but note that the style guide specifically mentions this as an acceptable way of doing assignments in piped workflows:

Why is this important?

The identation_linter() is part of the default set of linters, which means that anyone who is using this style of assignment will see lints from this linter. Add this to the fact that the new GHA workflow for linting produces errors if any lints are found.

This means that users are going to see build failures for writing tidyverse code in a style that the style guide explicitly says is acceptable. IMO, this is surprising and frustrating for the user.

We have a few options out of this quagmire:

• support this style of assignment (i.e., no lint from identation_linter())
• provide a parameter to identation_linter() to toggle on and off whether this assignment style should be linted
• ask the style guide to be revised

[AshesITR]

I think an optional argument (defaulting to tidyverse style) is in order.
The refactoring necessary for this isn't trivial, though :(

[MichaelChirico]

Q is, is this worth demoting indentation_linter() out of defaults for the next release? e.g. marking it as "WIP" and "slated-for-default", or do we think it's already consistent enough with the style guide to be released as a default with some known edge case like this?

[AshesITR]

I'd decide that based on how common the code style is that triggers the false-positive.
Also, not a lot to go on how exactly <- should suppress child indentations.
Surely, we still want these snippets to be considered "correct":

var <-
  function_call(
    arg1 +
      also_arg1,
    arg2
  )

Maybe only direct children infix tokens should be excluded?
That would be a fix that seems doable, so we ship 3.1.0 without the FP.

---

I've made a PR to fix this issue, so we can keep indentation_linter() in the defaults without worrying.
The XPath is a bit advanced, though. @MichaelChirico PTAL