Multiline if conditions

[lionel-]

I tried to apply usethis::use_tidy_style() on the purrr repo and I get a lot of changes from

  if (length(mode) > 1 &&
        !all(c("double", "integer") %in% mode)) {
    return(FALSE)
  }

to:

  if (length(mode) > 1 &&
    !all(c("double", "integer") %in% mode)) {
    return(FALSE)
  }

I don't have a preference between increasing the indent level after the boolean operator or aligning everything at the opening parenthesis. However aligning at the branch body like styler does seems very confusing.

[MichaelChirico]

I believe the current behavior is in line with the style guide:

https://style.tidyverse.org/syntax.html#long-lines

personally I prefer this styling for if clauses:

if (
  length(mode) > 1 &&
    !all(c("double", "integer") %in% mode)
) {
  return(FALSE)
}

this seems possibly related to tidyverse/style#173; probably needs to be raised upstream if you disagree w the rule.

[lionel-]

I believe the current behavior is in line with the style guide

This is not my interpretation of the style guide.

personally I prefer this styling for if clauses

ouch :-)

[lionel-]

This is not my interpretation of the style guide.

To be clear, the guide mentions function-call syntax but not if-condition syntax.

[hadley]

Seems like the same rules as https://style.tidyverse.org/functions.html#long-lines-1 should apply?

[lionel-]

Do you mean double-indent, which in this case coincides with indenting to open paren?

Since unfinished expressions after an operator normally create an indent level, I'm not sure these rules are applicable here. I think this is a separate rule that cross-cuts with these other rules:

foo <- a_long_name &&
  another_name

list(
  a_long_name &&
    another_name,
  bar
)

long_function_name <- function(
    a = a_long_argument &&
      another_argument,
    c = "another long argument") {
  # As usual code is indented by two spaces.
}

This implies

if (a_long_name &&
      another_name) {
  body
}

[lorenzwalthert]

Sorry I could not keep this brief...

Short version

We should have magrittr::or2() and magrittr::and2() (similar to extract2()) for multiple conditions that should be lazily evaluated and never use | or & (or their lazy counterparts) for multi-line evaluations:

should_continue <- or2(
  my(long = 'very', condition=1),
  someone_elses(condition['that'] + is / long)
)

if (!should_continue) { # or use or2() inline
  return(FALSE)
}

Why is that beneficial? To ensure multiple conditions that should be lazily evaluated have the same indention. Alternative is something like

# multi-line logical evaluation without if case already uggly
cond1(x_that + is(very) - long) &&
  cond2_with_a['long_variable']

# many (uggly) ways to format more complex case
if (cond1 &&
  cond2) {
  FALSE
}

if (
  cond1 &&
    cond2
) {
  FALSE
}

if (cond1 &&
    cond2) {
  FALSE
}

With or2() and and2(), no styling rule has to be extended to cover the case and to me it's aesthetically more pleasing. Until then, I suggest the code should be formatted like @MichaelChirico suggested, namely

if (
  length(mode) > 1 &&
    !all(c("double", "integer") %in% mode)
) {
  return(FALSE)
}

This is how {styler} source code is formatted currently. Introducing double indent is inconsistent with current rules and styling and adds more complexity for almost no gain IMO.

Long version

IIRC from a discussion with @hadley, he advised against multi-line if conditions in the first place, and suggested to re-write the if statement so it fits one line, e.g.

if (length(mode) > 1 & !all(c("double", "integer") %in% mode)) {
  return(FALSE)
}

becomes

not_scalar <- length(mode) > 1 
not_numeric <- !all(c("double", "integer") %in% mode)
if (not scalar & not_numeric) {
  return(FALSE)
}

This may make sense sometimes, but lazy evaluation is not supported for the second statement it is also quite verbose at times. Hence, I think we should support multi-line if statements.

Just looking at how {styler} handles all other cases of multi-line statments, it seems the opening braces don't have anything after them on the same line and closing braces don't have anything before them

call(
  x = 1, 
  y = 2
)

tryCatch(
  { 
    1 + NULL
  }, 
  error = function(...)
)

See also #549 for the last case and #485 in general for non-symmetric indention.

The exception is when multiple levels can be nested so two operators causing indention or unindention are on the same line (i.e. symmetric), but the indention of ( and { are not accumulated, we only indent by one level :

test_that('x', {
  my_test()
})

c(x = 1, y = c({
  x = 2
}))

Function declarations are a special case and to my knowledge the only case where we support

( code here
code here )

For that reason, I agree with @MichaelChirico that the preferred way is

if (
  length(mode) > 1 &&
    !all(c("double", "integer") %in% mode)
) {
  return(FALSE)
}

And that is also how I use it and how it's used in {styler}. Another option is to use syntactic sugar from {magrittr} (do we need and2()? and or2() for lazy evaluation?)

if (and(
  length(mode) > 1,
  !all(c("double", "integer") %in% mode)
)) {
  return(FALSE)
}

This has the advantage that first and second condition have the same indention. For that reason, I'd vote for a and2() and or2(), it's the most stylistically pleasing I think. That would also make other conditions look better, even outside if() clauses:

should_continue <- or2(
  my(long = 'very', condition=1),
  someone_elses(condition['that'] + is / long)
)

I hence do vote strongly against using double indent in this context. This just adds complexity for the end user (and {styler}) without gain. {styler} (like RStudio) does never indent more than one level per new line. Only exception will be the function definitions with double-indent, but that's not caused by a accumulation of operators that cause indent on one line, like in @lionel-'s example, where ( and & are triggering indent. {styler} only ever adds one level of indention, and I think that's good and should not be changed.

---

Conclusion: Add or2() and and2() to {magrittr} seems like a minor change that would resolve this case and even improve formatting beyond conditional statments for multi-line logical evaluations with && and ||

[lionel-]

Right, the preferred style is refactoring the condition out of the if. However styler can't enforce this, this is up to the author. And whether the author wants to use || or another operation like or2 is a separate issue that is also independent of styler.

The problem is that the current styler formatting indents the condition continuations on the same level as the branch bodies, which is confusing. Ideally it would be visually clear to what component of the if expression a line belongs.

For that reason, I agree with @MichaelChirico that the preferred way is

if (
  length(mode) > 1 &&
    !all(c("double", "integer") %in% mode)
) {
  return(FALSE)
}

This aesthetic looks strange to me. But I'm wondering why should styler make it more complicated? If && increases the indent level, then the minimal action to take is:

if (length(mode) > 1 &&
      !all(c("double", "integer") %in% mode)) {
  return(FALSE)
}

This is not great, but I think it's fine since the preferred style is to refactor that away with a variable.

[lorenzwalthert]

And whether the author wants to use || or another operation like or2 is a separate issue that is also independent of styler.

True. And this issue you rose is also not premarily a {styler} issue I think, because in my understanding, {styler} implements the tidyverse style guide (which does not specify the formatting in that case). I still think it should be discussed. Do you agree that magrittr::or2() and magrittr::and2() should be implemented? I think it makes sense in conjunction with a rule in the style guide like

Infix operators like |, && and friends should not used in multi-line statements. Instead, use their functional equivalents magrittr::or(), magrittr::and2() and the like.

---

But I'm wondering why should styler make it more complicated? If && increases the indent level, then the minimal action to take is:
[to indent with two levels on one line].

Because added indention per line is maximal one level (as described above) and it's the way it works interactively in RStudio, vs code, other editors and {styler}. This applies for all tokens, independent of whether it is a conditional statement:

1 + 1 + 2 + 
 3

call( 
  (
    ( 
      ((
        1
      ))
    )
  )
)

x && 3 + 
  2

call({
  1
})

if (x + 1 -
  3
)
  TRUE

function(x = f(
  new = 2
)) {
  FALSE
}

x[f(x, 
  y = 2
)]


x[f(x, y = n + 
  3
)]

I don't want to change that fundamental principle that the indention of multiple tokens on one line add up in {styler}. The only exception where indention can be more than one level per line I see to be implemented in {styler} is function declarations, although even there, I am not a big fan of it. But as explained above, this has a different origin (namely that in function declarations, the indention caused by ( is two levels). It's not that the indention of multiple tokens add up, as you suggest. So double indent in function declarations is still consistent with the rule that indention of multiple tokens on one line does not add up.

[lionel-]

other editors

This is not true for Emacs ESS.

Anyway, if that is contentious, I suggest implementing the default style in clang-format which is to not add an indent level after operators within a control statement. i.e. indent at open parenthesis.

[lionel-]

Here is an output from clang-format, it seems the rule is more general and applies within any set of parentheses.

long_long_long_long_long_long_long_long_long_long_long_long_name +
  long_long_long_long_long_long_long_long_long_long_long_long_name;

(long_long_long_long_long_long_long_long_long_long_long_long_name +
 long_long_long_long_long_long_long_long_long_long_long_long_name);

if (long_long_long_long_long_long_long_long_long_long_long_long_name +
    (long_long_long_long_long_long_long_long_long_long_long_long_name +
     long_long_long_long_long_long_long_long_long_long_long_long_name) +
    long_long_long_long_long_long_long_long_long_long_long_long_name) {
  return NULL;
}

[hadley]

I don't think we're likely to implement and2() or or2(), and we're unlikely to make any further changes to magrittr, given |>.

[lionel-]

Regarding

Because added indention per line is maximal one level

I think that's the case in the styles I proposed. There is only one additional indent level in:

if (foo +
      bar)

And here there is none:

if (foo +
    bar)

The thing is that in both these styles the open parenthesis overrides the current indentation level for incomplete expressions.

This doesn't seem to be an uncommon rule in editors (I mostly use Emacs but clang-format does the same).

[lorenzwalthert]

And here there is none:

I don't understand what you mean. Line 1 of your example has zero leading spaces, line 2 has 4 spaces. That's two indention levels if we assume one level has two spaces. I meant we only add one indention level per line, so there is zero spaces in line 1, then two spaces in line 2.

if (x +  # zero
  3) { # two
  # two again
}

Other option is to even have zero for line 2, as the ) unindents code:

call(
  1
  ) # we don't do this, `)`  removes the indention here, leaving it at indent 0

Like this:

if (x +  # zero
3) { # zero
  # two again
}

But this seems even worse.

As pointed out earlier, using infix operators in multi-line contex makes things unnecessary complex and my point of view is that we should avoid expressions with infix operators that span over multiple lines altogether, e.g. with {magrittr}:

if (add(
  x, 
  3
)) {
  # stuff
}

[lionel-]

Line 1 of your example has zero leading spaces, line 2 has 4 spaces.

Line 2 has 6 spaces. The 4 spaces of the indentation level implied by the opening paren, and the 2 spaces of the additional indent level created by the incomplete expression.

In the second example, line 2 has 4 spaces all created by the open paren.

In both these styles the open parenthesis overrides the current indentation level for incomplete expressions.

[lorenzwalthert]

Ok, I was referring to your fist code snippet. As I said multiple times, I don't see why we should accumulate indention. Neither RStudio nor VS code does that (at least not by default), so the vast majority of our users is already used to the current behaviour of {styler}.

In the second example, line 2 has 4 spaces all created by the open paren.

Ok, so when does an opening parenthesis cause two spaces of indention and when 4? To me, it makes sense that it always causes two (unless there was another token on that line that already caused indention, it causes no additional indention).

[lionel-]

I feel like we are talking past each other. Let's just close this issue, we shouldn't be spending too much time on this unimportant edge case.

[lorenzwalthert]

I feel so too, sorry if I did not get what you wanted to bring across. If you find other (easier) bugs/problems with {styler}, I am happy to discuss.