Unify bracing-related linters / parentheses / if-else clauses?

[MichaelChirico]

We currently have a variegated list of linters related to bracing and parentheses:

• closed_curly_linter()
• else_same_line_linter()
• function_brace_linter()
• function_left_parentheses_linter()
• if_else_match_braces_linter()
• open_curly_linter()
• paren_body_linter()
• paren_brace_linter()
• spaces_inside_linter()
• spaces_left_parentheses_linter()

Should we consolidate any of these?

[AshesITR]

Conceptuall, I can see linters related to the style of function definitions and linters related to flow control statements.
The many stylistic difference between the two is the spacing of the control condition (requires a space after the keyword) vs. the spacing of the argument list (forbids space after the function keyword).

The spaces_inside_linter() doesn't really fit this description imo.
paren_body_linter() could be included in a function_style_linter() but isn't related to braces.

A brace_linter() could unify

• closed_curly_linter(), keeping the allow_single_line = FALSE option
• else_same_line_linter()
• function_brace_linter() -- btw: the lint type should probably be "style".
• The FUNCTION part of function_left_parentheses_linter(), but not the SYMBOL_FUNCTION_CALL part.
• `if_else_match_braces_linter()
• open_curly_linter(), keeping the allow_single_line = FALSE option and adding an option to allow C-style (i.e. { must always be on its own line -- NB though that else can't be on a different line than } in R, so this should probably only apply to function())
• paren_brace_linter(), which should also lint if line breaks are used as spacing unless aforementioned C-style option is on.
• spaces_left_parentheses_linter()

Alternatively, we could unify to two linters

1.function_style_linter() with options allow_single_line = FALSE, c_style_braces = FALSE, allow_space_before_args = FALSE and allow_braceless_multiline = FALSE
2. flow_style_linter() with options allow_single_line = FALSE, c_style_braces = FALSE and allow_braceless_multiline = FALSE

NB that since many of these linters are default in 2.0.1, custom linter configs with, e.g. with_defaults(open_curly_linter = NULL) will currently just stop working as expected without a warning.
It might thus be helpful to include a check in linters_with_tags() and with_defaults() that elements assigned to NULL are actually in the default list and warn otherwise, so this doesn't go undetected.

What are your thoughts?

[MichaelChirico]

NB that since many of these linters are default in 2.0.1, custom linter configs with, e.g. with_defaults(open_curly_linter = NULL) will currently just stop working as expected without a warning.

great point.

It might thus be helpful to include a check in linters_with_tags() and with_defaults() that elements assigned to NULL are actually in the default list and warn otherwise, so this doesn't go undetected.

indeed. and this is a generally useful test, too, not just for deprecations.

function_style_linter() + flow_style_linter()

IMO the UX is not great here -- I read those and only get a vague sense of what it's for. braces_linter() has a very clear purpose -- where I go if I want to control how lintr governs the use of { / } in my code.

[AshesITR]

Filed #1049 for the NULL check

[MichaelChirico]

Copying comment from the dup'd issue:

While working on this issue:
The wording of the function linter should be changed to "should" instead of "must":

Any function spanning multiple lines must use curly braces.

[AshesITR]

@MichaelChirico I've completed a PR chain to do this, but left out function_left_parentheses_linter() and spaces_left_parentheses_linter(), because those two actually lint parentheses and not braces, and especially the latter is already fairly complex as-is.

LMK what you think.
A few features slipped in as well, to make behaviour more consistent:

open_curly_linter() linted trailing whitespace as well, which is unnecessary:

lintr::lint(
  text = "a <- function() {  \n}",
  linters = list(
    trailing_whitespace_linter(),
    open_curly_linter()
  )
)
#> <text>:1:17: style: Opening curly braces should never go on their own line and should always be followed by a new line.
#> a <- function() {  
#>                 ^
#> <text>:1:18: style: Trailing whitespace is superfluous.
#> a <- function() {  
#>                  ^~

paren_brace_linter() only linted ){, but not else{ and repeat{. I've added these and changed the message accordingly.

Once the merging is complete, adding a c_style_braces = FALSE option to enforce { and } to always go on their own line could be done in a separate PR.

[MichaelChirico]

Sorry, just saw the comment here.

[I] left out function_left_parentheses_linter() and spaces_left_parentheses_linter(), because those two actually lint parentheses and not braces...

SGTM. Do we want a parentheses_linter() then for parentheses-specific rules?