Allow `{ }` or `{}` in brace_linter independently of allow_single_line?

[MichaelChirico]

I am seeing some code linted by brace_linter that wasn't throwing a lint in 2.0.1, e.g.

function(...) {} # e.g. as the error= in tryCatch()
if (...) { ... } else {}
tryCatch(..., finally = {})
switch(x, a = {}, b = 2) # NB: different than switch(x, a = , b = 2)
while (...) {}

Should we allow any/all of the above to be used without linting? If so, should we require a uniform # of spaces, e.g. {} is OK but { } is not?

[AshesITR]

The empty expression should be {}.
NB NULL is equivalent and clearer in intent I think.

We should allow {} if allow_single_line == TRUE.

[MichaelChirico]

I'm not sure. If the expression is just thrown away, NULL is a distraction. I think switch(x, a = NULL, b = 1) is better than switch(x, a = {}, b = 1), but while (process_is_running()) {} is better than while (process_is_running()) NULL.

[AshesITR]

Well, no disagreement on what we should do then :)
Although while (might_block_forever()) {} is a code smell in and of itself 😉

[MichaelChirico]

while (might_block_forever()) {} is a code smell in and of itself wink

A bit, but we can't really distinguish it from while (function_with_internal_timeout_mechanism()) {} so I leave it to the downstream code reviewers to judge. e.g. Such loops are called out as OK in the Google C++ style guide:

https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

We should allow {} if allow_single_line == TRUE.

Is that an "if and only if"?

FWIW, styler treats empty expressions differently:

library(styler)

style_text("while (TRUE) { }")
# while (TRUE) { }
style_text("while (TRUE) {}")
# while (TRUE) {}
style_text("while (TRUE) { 1 }")
# while (TRUE) {
#   1
# }

And personally I don't think splitting {} into different lines is ever a good idea. So I think the default should be to allow {} and { } regardless of allow_single_line.

Filed tidyverse/style#191 for potential clarity from the Tidyverse guide.

[AshesITR]

(Sorry I accidentally hit edit instead of comment. Is there a way to revert?)

We should allow {} if allow_single_line == TRUE.

Is that an "if and only if"?

Yes. My reasoning is if you don't want to allow_single_line usages, you have a point and can write code that obeys the style in all situations:

while (function_that_stops_after_some_retries()) {
  # comment why we allow an empty loop here
}

# The empty function actually returns NULL, so make it explicit:
function() NULL

NB how control flow (while) should be "highlighted" as per the tidyverse style guide also.

[MichaelChirico]

I don't really agree with the need force awkwardly shoehorning a comment in where there was none before... but on the other hand, I originally opened this issue because I thought it was a regression from 2.0.1 behavior that these lints were showing up.

But all of the examples in the issue header generate at least open_curly_linter() hits, and closed_curly_linter() hits except for }) cases, and allow_single_line is already working as described in the comments here. So there's no regression, and users like me have an out with allow_single_line = TRUE.

So I am happy to just table this for now, until there's any feedback on the style guide stance for these, or user feedback after release.

Perhaps @lorenzwalthert could chime in if there's some discussion that took place in styler relevant to this decision too.

[lorenzwalthert]

Thanks @MichaelChirico. I think first and foremost, we should improve consistency between styler and lintr. This goes beyond just this particular question. I don’t personally use lintr so I don’t have a feeling for it but I got some feedback that people turn off either one with precommit to avoid headache. As for this specific case, I think there is some discussion in r-lib/styler#256, but it’s mainly my and Kirill’s opinion.

If you have one expression in curly braces, I think you can omit the curly braces, no?

call(x = {call2()}
# equivalent
call(x = call2())

That's why I'd only use them for multi line expressions like

call(
  x = {
    a = 1 + 3
    call2(a, 44)
  }
)

Let's wait for tidyverse/style#191, I don't have strong preferences.

[MichaelChirico]

If you have one expression in curly braces, I think you can omit the curly braces, no?

Essentially yes, and we have a linter in the works for this, but NB {testthat} enforces using {expr} even when not needed because there's a funny thing w the R parser where helpful source info is not retained unless the expression is in braces, so they throw a warning otherwise:

test_that("a simple test", expect_true(TRUE))
# Test passed 🥳
# Warning message:
# The `code` argument to `test_that()` must be a braced expression to get accurate file-line information for failures. 

[MichaelChirico]

I think we should follow {styler} on this issue for now and lint neither { } nor {} by default, until there's any update in the style guide issue.