Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add param function_braces to brace_linter() #2240

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f37b35b
Add param `function_braces` to `brace_linter()`
salim-b Oct 18, 2023
acf4370
refactor, add option not_inline
AshesITR Oct 19, 2023
de6989f
update NEWS bullet
AshesITR Oct 19, 2023
1494755
Fix typos
salim-b Oct 19, 2023
63ac52f
Roxygenize
salim-b Nov 10, 2023
25f1ed8
Merge branch 'main' into allow-fn-brace
salim-b Mar 12, 2024
54c1ed3
Move NEWS item, clarify it
MichaelChirico Mar 12, 2024
555b192
Update R/brace_linter.R
salim-b Mar 12, 2024
a4b54da
Update R/brace_linter.R
salim-b Mar 12, 2024
229993a
Use list in roxygen doc
salim-b Mar 13, 2024
5eaae61
Use 'true' in XPath for clarity
MichaelChirico Mar 13, 2024
34af80d
rename param `function_braces` to `function_bodies`
salim-b Mar 13, 2024
89cd471
Merge branch 'allow-fn-brace' of github.com:salim-b/lintr into allow-…
salim-b Mar 13, 2024
ecbe9b6
Revert "Use 'true' in XPath for clarity"
salim-b Mar 13, 2024
54d257f
Fix last merge
salim-b Mar 13, 2024
3e13651
Update R/brace_linter.R
salim-b Mar 13, 2024
9e27450
Roxygenize
salim-b Mar 13, 2024
13bd010
Merge branch 'allow-fn-brace' of github.com:salim-b/lintr into allow-…
salim-b Mar 13, 2024
bd7afa7
Update tests
salim-b Mar 13, 2024
9e1568c
Merge branch 'main' into allow-fn-brace
salim-b Mar 25, 2024
9fe4c56
Merge branch 'main' into allow-fn-brace
salim-b Jul 13, 2024
f4a3a70
grammar
MichaelChirico Aug 6, 2024
4223eea
use 'true' over '1' for boolean literal in XPath
MichaelChirico Aug 6, 2024
7620e37
slightly improve readability of XPath
MichaelChirico Aug 6, 2024
2dc893e
Merge branch 'main' into allow-fn-brace
MichaelChirico Aug 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
* `todo_comment_linter()` has a new argument `except_regex` for setting _valid_ TODO comments, e.g. for forcing TODO comments to be linked to GitHub issues like `TODO(#154)` (#2047, @MichaelChirico).
* `vector_logic_linter()` is extended to recognize incorrect usage of scalar operators `&&` and `||` inside subsetting expressions like `dplyr::filter(x, A && B)` (#2166, @MichaelChirico).
* `any_is_na_linter()` is extended to catch the unusual usage `NA %in% x` (#2113, @MichaelChirico).
* `brace_linter()`' has a new argument `function_bodies` (default `"multi_line"`) which controls whether to require function bodies to be wrapped in curly braces, with the options `"always"`, `"multi_line"` (only require curly braces when a function body spans over multiple lines), `"not_inline"` (only require curly braces when a function body starts on a new line) and `"never"` (#1807, #2240, @salim-b).
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
* `make_linter_from_xpath()` errors up front when `lint_message` is missing (instead of delaying this error until the linter is used, #2541, @MichaelChirico).
* `paste_linter()` is extended to recommend using `paste()` instead of `paste0()` for simply aggregating a character vector with `collapse=`, i.e., when `sep=` is irrelevant (#1108, @MichaelChirico).
* `expect_no_lint()` was added as new function to cover the typical use case of expecting no lint message, akin to the recent {testthat} functions like `expect_no_warning()` (#2580, @F-Noelle).
Expand Down
51 changes: 39 additions & 12 deletions R/brace_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,14 @@
#' - Closing curly braces in `if` conditions are on the same line as the corresponding `else`.
#' - Either both or neither branch in `if`/`else` use curly braces, i.e., either both branches use `{...}` or neither
#' does.
#' - Functions spanning multiple lines use curly braces.
#' - Function bodies are wrapped in curly braces.
#'
#' @param allow_single_line if `TRUE`, allow an open and closed curly pair on the same line.
#' @param allow_single_line If `TRUE`, allow an open and closed curly pair on the same line.
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
#' @param function_bodies Whether to require function bodies to be wrapped in curly braces. One of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Whether" kind of implies a binary choice

Suggested change
#' @param function_bodies Whether to require function bodies to be wrapped in curly braces. One of
#' @param function_bodies Character string governing when function bodies without curly braces should generate lints. One of

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as @AshesITR points out, I think we should be careful here to specify "function body" for all the rules, since a function definition includes the parameters and those can take up many lines even if the function itself doesn't (not that we expect to see that except in strange cases).

#' - `"always"` to require braces for all function definitions, including inline functions,
#' - `"not_inline"` to require braces when a function body does not start on the same line as its signature,
#' - `"multi_line"` (the default) to require braces when a function definition spans multiple lines,
#' - `"never"` to never require braces in function bodies.
#'
#' @examples
#' # will produce lints
Expand Down Expand Up @@ -50,7 +55,10 @@
#' - <https://style.tidyverse.org/syntax.html#indenting>
#' - <https://style.tidyverse.org/syntax.html#if-statements>
#' @export
brace_linter <- function(allow_single_line = FALSE) {
brace_linter <- function(allow_single_line = FALSE,
function_bodies = c("multi_line", "always", "not_inline", "never")) {
function_bodies <- match.arg(function_bodies)

xp_cond_open <- xp_and(c(
# matching } is on same line
if (isTRUE(allow_single_line)) {
Expand Down Expand Up @@ -124,7 +132,25 @@ brace_linter <- function(allow_single_line = FALSE) {
# TODO(#1103): if c_style_braces is TRUE, this needs to be @line2 + 1
xp_else_same_line <- glue("//ELSE[{xp_else_closed_curly} and @line1 != {xp_else_closed_curly}/@line2]")

xp_function_brace <- "(//FUNCTION | //OP-LAMBDA)/parent::expr[@line1 != @line2 and not(expr[OP-LEFT-BRACE])]"
if (function_bodies != "never") {
xp_cond_function_brace <- switch(
function_bodies,
always = "1",
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
multi_line = "@line1 != @line2",
not_inline = "@line1 != expr/@line1"
)

xp_function_brace <- glue(
"(//FUNCTION | //OP-LAMBDA)/parent::expr[{xp_cond_function_brace} and not(expr[OP-LEFT-BRACE])]"
MichaelChirico marked this conversation as resolved.
Show resolved Hide resolved
)

msg_function_brace <- switch(
function_bodies,
always = "Wrap function bodies in curly braces.",
multi_line = "Wrap multi-line function bodies in curly braces.",
not_inline = "Wrap function bodies starting on a new line in curly braces."
)
}

# if (x) { ... } else if (y) { ... } else { ... } is OK; fully exact pairing
# of if/else would require this to be
Expand Down Expand Up @@ -188,15 +214,16 @@ brace_linter <- function(allow_single_line = FALSE) {
lint_message = "`else` should come on the same line as the previous `}`."
)
)

lints <- c(
lints,
xml_nodes_to_lints(
xml_find_all(xml, xp_function_brace),
source_expression = source_expression,
lint_message = "Use curly braces for any function spanning multiple lines."
if (function_bodies != "never") {
lints <- c(
lints,
xml_nodes_to_lints(
xml_find_all(xml, xp_function_brace),
source_expression = source_expression,
lint_message = msg_function_brace
)
)
)
}

lints <- c(
lints,
Expand Down
17 changes: 14 additions & 3 deletions man/brace_linter.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 46 additions & 9 deletions tests/testthat/test-brace_linter.R
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested new test case:

foo <- function(x) ({
  x + 1
})

(I think it should lint, this is an unusual way to declare a function that should be discouraged in favor of plain {.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, something like:

foo <- function(x) { x +
  4 }

Is slightly different from the current set of cases & worth including as a regression test I think.

Original file line number Diff line number Diff line change
Expand Up @@ -299,25 +299,62 @@ test_that("brace_linter lints else correctly", {
})

test_that("brace_linter lints function expressions correctly", {
linter <- brace_linter()
expect_lint("function(x) 4", NULL, linter)
msg_always <- rex::rex("Wrap function bodies in curly braces.")
msg_multi_line <- rex::rex("Wrap multi-line function bodies in curly braces.")
msg_not_inline <- rex::rex("Wrap function bodies starting on a new line in curly braces.")

linter_always <- brace_linter(function_bodies = "always")
linter_multi_line <- brace_linter(function_bodies = "multi_line")
linter_not_inline <- brace_linter(function_bodies = "not_inline")
linter_never <- brace_linter(function_bodies = "never")

lines <- trim_some("
function(x) {
x + 4
}
")
expect_lint(lines, NULL, linter)
expect_lint(lines, NULL, linter_always)
expect_lint(lines, NULL, linter_multi_line)
expect_lint(lines, NULL, linter_not_inline)
expect_lint(lines, NULL, linter_never)

lints_single_line <- list(
rex::rex("Opening curly braces should never go on their own line and should always be followed by a new line."),
rex::rex("Closing curly-braces should always be on their own line, unless they are followed by an else.")
)
expect_lint("function(x) { x + 4 }", lints_single_line, linter_always)
expect_lint("function(x) { x + 4 }", lints_single_line, linter_multi_line)
expect_lint("function(x) { x + 4 }", lints_single_line, linter_not_inline)
expect_lint("function(x) { x + 4 }", lints_single_line, linter_never)
# using function_bodies = "always" without allow_single_line = TRUE prohibits inline function definitions:
expect_lint(
"function(x) { x + 4 }",
NULL,
brace_linter(allow_single_line = TRUE, function_bodies = "always")
)

expect_lint("function(x) x + 4", msg_always, linter_always)
expect_lint("function(x) x + 4", NULL, linter_multi_line)
expect_lint("function(x) x + 4", NULL, linter_not_inline)
expect_lint("function(x) x + 4", NULL, linter_never)

lines <- trim_some("
function(x) x +
4
")
expect_lint(lines, msg_always, linter_always)
expect_lint(lines, msg_multi_line, linter_multi_line)
expect_lint(lines, NULL, linter_not_inline)
expect_lint(lines, NULL, linter_never)

lines <- trim_some("
function(x)
x+4
x + 4
")
expect_lint(
lines,
rex::rex("Use curly braces for any function spanning multiple lines."),
linter
)
expect_lint(lines, msg_always, linter_always)
expect_lint(lines, msg_multi_line, linter_multi_line)
expect_lint(lines, msg_not_inline, linter_not_inline)
expect_lint(lines, NULL, linter_never)
})

test_that("brace_linter lints if/else matching braces correctly", {
Expand Down
Loading