Redundant line breaks in function header not removed when formatting

[michaelquinn32]

After resolving #541, tryCatch blocks now terminate in a new line.

code <- "tryCatch({
  my_options <- options()
  print('hello!')
},
error = function(e) e)
"
styler::style_text(code)
#> tryCatch(
#>   {
#>    my_options <- options()
#>    print("hello!")
#>  },
#>  error = function(e) e
#> )

This seems like one good case from #125, but there are examples in that issue where the original formatting was considered correct.

Similarly, function signatures that used a wrapped style for arguments also end in an empty new line.

code <- "function(
    my_indendented_arg, and_another) {
  print('hello')
}"
styler::style_text(code)
#> function(
#>          my_indendented_arg, and_another) {
#>   print("hello")
#> }

These seem to be related as cases where there are empty new lines after an opening paren.

Thanks!

[lorenzwalthert]

I think I can't follow.

First, I believe indeed that some formatting previously considered correct (also from #125) is now considered not correct anymore, for example

tryCatch({
  my_options <- options()
  print("hello!")
},
error = function(e) e
)

Is no longer desired and is now turned into:

tryCatch(
  {
    my_options <- options()
    print("hello!")
  },
  error = function(e) e
)

This is intentional and not related to #541, it was introduced in #543.

I believe the second issue you raise could be re-formulated with redundant line breaks in function formals / signatures / headers are not removed. I.e.

function(
         my_indendented_arg, and_another) {
  print("hello")
}

Will be returned unchanged by styler. This is indeed not nice, but not new, it's been the same in styler 1.1.1 (current CRAN release).

If my understanding is correct, then I'd suggest renaming the issue accordingly.

[michaelquinn32]

Thanks for the feedback!

If the first case is intentional, then they aren't related. I'll rename the issue to focus on redundant line breaks.

[lorenzwalthert]

So for

code <- "function(
    my_indendented_arg, and_another) {
  print('hello')
}"

Do you think the preferred output is

code <- "function(my_indendented_arg, 
                  and_another) {
  print('hello')
}"

or

code <- "function(my_indendented_arg, and_another) {
  print('hello')
}"

I think with the first one, we don't do anything wrong.

[michaelquinn32]

Ideally, the preference between the two would depend on the character length of option 2. Fixing long lines has been some work, and I know that this is an issue that you're addressing.

This means that the first option is fine for this case. In all likelihood, the indented signature was probably used to address a line that was too long.

[michaelquinn32]

@jimhester The tryCatch behavior described above is now throwing lintr errors:

tryCatch(
  {
    my_options <- options()
    print("hello!")
  },
  error = function(e) e
)

Since it places an open curly brace on its own line. Should I open a PR for lintr?

[jimhester]

tryCatch(
  {
    my_options <- options()
    print("hello!")
  },
  error = function(e) e
)

Is quite ugly, I have no intention of changing how lintr behaves with regards to this, if you want this you can edit your default linters to accept this style.

[lorenzwalthert]

Ok, interesting. @jimhester how would you format this? After debating with @krlmlr, he finally convinced me in #428 that this is preferred so I changed it in styler #543 -.-

I think we should formalize a rule for braces in function calls in https://github.com/tidyverse/style and then make sure styler and lintr are on par. I'll open an issue.

There is already a tryCatch() example in Section 2.4 of the tidyverse style guide:

tryCatch(
  {
    x <- scan()
    cat("Total: ", sum(x), "\n", sep = "")
  },
  interrupt = function(e) {
    message("Aborted by user")
  }
)

So I assume this is the preferred style according to the tidyverse style guide, which matches styler's current behaviour.

[jimhester]

I always format them like this, I don't really agree with what is in the style guide.

tryCatch({
    my_options <- options()
    print("hello!")
  },
  error = function(e) e
)

[lorenzwalthert]

Ok, one reason we did not opt for this is because styler's is implementing the logic that you can only indent one level every new line (I think, can't exactly remember).

[jimhester]

If you do want the arguments to match up I think doing so explicitly looks better, and also won't have lints with lintr.

tryCatch(
  expr = {
    x <- scan()
    cat("Total: ", sum(x), "\n", sep = "")
  },
  interrupt = function(e) {
    message("Aborted by user")
  }
)

[lorenzwalthert]

So how do you indent test_that() calls then?

test_that("here", {
    "two-levels"
})

[krlmlr]

I fully stand behind the current implementation in styler. For me it's important that the code style applied automatically is nearly identical to what would happen when you type the same code manually, because this decreases the burden when editing the code.

For now it seems that users of both lintr and styler need

tryCatch(
  expr = {
    ...
  }
)

. The following produces lints

tryCatch(
  {
     ...
  }
)

while the latter won't be produced by styler currently:

tryCatch({
    ...
  }
)

This looks like a situation where everybody can find their peace. Let's focus on function(\n...) here and discuss tryCatch() elsewhere.