Skip to content

Commit b4d39bb

Browse files
Unified brace_linter (#1092)
* create brace_linter based on XPath - deprecate closed_curly_linter - add brace_linter to defaults instead of closed_curly_linter - add breaking change to NEWS * fix missing newline, update warning tests for semicolon_terminator_linter * test for closed_curly_linter warning and make tests more silent * remove c_style_braces for now * document() * allow ]}, update NEWS, incorporate feedback, fix lint * delete else_same_line_linter and merge it into brace_linter (#1093) * delete else_same_line_linter and merge it into brace_linter * delete function_brace_linter and merge it into brace_linter (#1094) * delete function_brace_linter and merge it into brace_linter * delete if_else_match_braces_linter and merge it into brace_linter (#1095) * delete if_else_match_braces_linter and merge it into brace_linter * deprecate open_curly_linter and merge it into brace_linter (#1096) * deprecate open_curly_linter - remove open_curly_linter from defaults - refactor to XPath based approach - no longer lint trailing whitespace (there's a separate linter for that) * merge paren_brace_linter into brace_linter (#1097) * deprecate paren_brace_linter - remove paren_brace_linter from defaults - extend to else{ and repeat{ * `code` Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> * add explicit test for different behaviour compared to closed_curly_linter Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com> Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
1 parent 56fec1b commit b4d39bb

34 files changed

+610
-394
lines changed

DESCRIPTION

+1-3
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ Collate:
5454
'any_is_na_linter.R'
5555
'assignment_linter.R'
5656
'backport_linter.R'
57+
'brace_linter.R'
5758
'cache.R'
5859
'class_equals_linter.R'
5960
'closed_curly_linter.R'
@@ -67,7 +68,6 @@ Collate:
6768
'declared_functions.R'
6869
'deprecated.R'
6970
'duplicate_argument_linter.R'
70-
'else_same_line_linter.R'
7171
'equals_na_linter.R'
7272
'exclude.R'
7373
'expect_comparison_linter.R'
@@ -82,11 +82,9 @@ Collate:
8282
'expect_type_linter.R'
8383
'extract.R'
8484
'extraction_operator_linter.R'
85-
'function_brace_linter.R'
8685
'function_left_parentheses.R'
8786
'get_source_expressions.R'
8887
'ids_with_token.R'
89-
'if_else_match_braces_linter.R'
9088
'ifelse_censor_linter.R'
9189
'implicit_integer_linter.R'
9290
'infix_spaces_linter.R'

NAMESPACE

+1-3
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ export(assignment_linter)
1919
export(available_linters)
2020
export(available_tags)
2121
export(backport_linter)
22+
export(brace_linter)
2223
export(checkstyle_output)
2324
export(class_equals_linter)
2425
export(clear_cache)
@@ -34,7 +35,6 @@ export(default_settings)
3435
export(default_undesirable_functions)
3536
export(default_undesirable_operators)
3637
export(duplicate_argument_linter)
37-
export(else_same_line_linter)
3838
export(equals_na_linter)
3939
export(expect_comparison_linter)
4040
export(expect_identical_linter)
@@ -49,11 +49,9 @@ export(expect_s4_class_linter)
4949
export(expect_true_false_linter)
5050
export(expect_type_linter)
5151
export(extraction_operator_linter)
52-
export(function_brace_linter)
5352
export(function_left_parentheses_linter)
5453
export(get_source_expressions)
5554
export(ids_with_token)
56-
export(if_else_match_braces_linter)
5755
export(ifelse_censor_linter)
5856
export(implicit_integer_linter)
5957
export(infix_spaces_linter)

NEWS.md

+13-2
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,15 @@
99
* Consistent access to linters through a function call, even for linters without parameters (#245, @fangly, @AshesITR, and @MichaelChirico)
1010
* Removed deprecated functions `absolute_paths_linter`, `camel_case_linter`, `multiple_dots_linter`, `snake_case_linter`, and `trailing_semicolons_linter`. They have been marked as deprecated since v1.0.1, which was released in 2017.
1111
* Rename `semicolon_terminator_linter` to `semicolon_linter` for better consistency. `semicolon_terminator_linter` survives but is marked for deprecation. The new linter also has a new signature, taking arguments `allow_compound` and `allow_trailing` to replace the old single argument `semicolon=`, again for signature consistency with other linters.
12+
* Combined several curly brace related linters into a new `brace_linter`:
13+
+ `closed_curly_linter()`, also allowing `}]` in addition to `})` and `},` as exceptions.
14+
* Combined several curly brace related linters into a new `brace_linter` (#1041, @AshesITR):
15+
+ `closed_curly_linter()`
16+
+ `open_curly_linter()`, no longer linting unnecessary trailing whitespace
17+
+ `paren_brace_linter()`, also linting `if`/`else` and `repeat` with missing whitespace
18+
+ Require `else` to come on the same line as the preceding `}`, if present (#884, @michaelchirico)
19+
+ Require functions spanning multiple lines to use curly braces (@michaelchirico)
20+
+ Require balanced usage of `{}` in `if`/`else` conditions (@michaelchirico)
1221
* The `...` arguments for `lint()`, `lint_dir()`, and `lint_package()` have promoted to an earlier position to better match the [Tidyverse design principal](https://design.tidyverse.org/args-data-details.html) of data->descriptor->details. This change enables passing objects to `...` without needing to specify non-required arguments, e.g. `lint_dir("/path/to/dir", linter())` now works without the need to specify `relative_path`. This affects some code that uses positional arguments. (#935, @michaelchirico)
1322
+ For `lint()`, `...` is now the 3rd argument, where earlier this was `cache=`
1423
+ For `lint_dir()` and `lint_package()`, `...` is now the 2nd argument, where earlier this was `relative_path=`
@@ -117,7 +126,6 @@ function calls. (#850, #851, @renkun-ken)
117126
+ Extended for #1067 to exclude `$` extractions like `expect_equal(x$"key", 2)`
118127
* `expect_identical_linter()` Require usage of `expect_identical()` by default, and `expect_equal()` only by exception
119128
* `expect_comparison_linter()` Require usage of `expect_gt(x, y)` over `expect_true(x > y)` and similar
120-
* `if_else_match_braces_linter()` Require balanced usage of `{}` in `if`/`else` conditions
121129
* `vector_logic_linter()` Require use of scalar logical operators (`&&` and `||`) inside `if()` conditions and similar
122130
* `any_is_na_linter()` Require usage of `anyNA(x)` over `any(is.na(x))`
123131
* `class_equals_linter()` Prevent comparing `class(x)` with `==`, `!=`, or `%in%`, where `inherits()` is typically preferred
@@ -131,7 +139,10 @@ function calls. (#850, #851, @renkun-ken)
131139
* `nested_ifelse_linter()` Prevent nested calls to `ifelse()` like `ifelse(A, x, ifelse(B, y, z))`, and similar
132140
* `condition_message_linter` Prevent condition messages from being constructed like `stop(paste(...))` (where just `stop(...)` is preferable)
133141
* `redundant_ifelse_linter()` Prevent usage like `ifelse(A & B, TRUE, FALSE)` or `ifelse(C, 0, 1)` (the latter is `as.numeric(!C)`)
134-
* `else_same_line_linter()` Require `else` to come on the same line as the preceding `}`, if present
142+
* Extensions to `brace_linter()`
143+
+ Require `else` to come on the same line as the preceding `}`, if present
144+
+ Require balanced usage of `{}` in `if`/`else` conditions
145+
+ Require functions spanning multiple lines to use curly braces
135146
* `unreachable_code_linter()` Prevent code after `return()` and `stop()` statements that will never be reached (extended for #1051 thanks to early user testing, thanks @bersbersbers!)
136147
* `regex_subset_linter()` Require usage of `grep(ptn, x, value = TRUE)` over `x[grep(ptn, x)]` and similar
137148
* `consecutive_stopifnot_linter()` Require consecutive calls to `stopifnot()` to be unified into one

R/brace_linter.R

+159
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
#' Brace linter
2+
#'
3+
#' Perform various style checks related to placement and spacing of curly braces:
4+
#'
5+
#' - Opening curly braces are never on their own line and are always followed by a newline.
6+
#' - Opening curly braces have a space before them.
7+
#' - Closing curly braces are on their own line unless they are followed by an `else`.
8+
#' - Closing curly braces in `if` conditions are on the same line as the corresponding `else`.
9+
#' - Either both or neither branch in `if`/`else` use curly braces, i.e., either both branches use `{...}` or neither
10+
#' does.
11+
#' - Functions spanning multiple lines use curly braces.
12+
#'
13+
#' @param allow_single_line if `TRUE`, allow an open and closed curly pair on the same line.
14+
#'
15+
#' @evalRd rd_tags("brace_linter")
16+
#' @seealso [linters] for a complete list of linters available in lintr. \cr
17+
#' <https://style.tidyverse.org/syntax.html#indenting> \cr
18+
#' <https://style.tidyverse.org/syntax.html#if-statements>
19+
#' @export
20+
brace_linter <- function(allow_single_line = FALSE) {
21+
Linter(function(source_expression) {
22+
if (length(source_expression$xml_parsed_content) == 0L) {
23+
return(list())
24+
}
25+
26+
lints <- list()
27+
28+
xp_cond_open <- xp_and(c(
29+
# matching } is on same line
30+
if (isTRUE(allow_single_line)) {
31+
"(@line1 != following-sibling::OP-LEFT-BRACE/@line1)"
32+
},
33+
# double curly
34+
"not(
35+
(@line1 = parent::expr/preceding-sibling::OP-LEFT-BRACE/@line1) or
36+
(@line1 = following-sibling::expr/OP-LEFT-BRACE/@line1)
37+
)"
38+
))
39+
40+
# TODO (AshesITR): if c_style_braces is TRUE, invert the preceding-sibling condition
41+
xp_open_curly <- glue::glue("//OP-LEFT-BRACE[
42+
{ xp_cond_open } and (
43+
not(@line1 = parent::expr/preceding-sibling::*/@line2) or
44+
@line1 = following-sibling::*[1][not(self::COMMENT)]/@line1
45+
)
46+
]")
47+
48+
lints <- c(lints, lapply(
49+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_open_curly),
50+
xml_nodes_to_lint,
51+
source_file = source_expression,
52+
lint_message = paste(
53+
"Opening curly braces should never go on their own line and",
54+
"should always be followed by a new line."
55+
)
56+
))
57+
58+
xp_open_preceding <- "parent::expr/preceding-sibling::*[1][self::OP-RIGHT-PAREN or self::ELSE or self::REPEAT]"
59+
60+
xp_paren_brace <- glue::glue("//OP-LEFT-BRACE[
61+
@line1 = { xp_open_preceding }/@line1
62+
and
63+
@col1 = { xp_open_preceding }/@col2 + 1
64+
]")
65+
66+
lints <- c(lints, lapply(
67+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_paren_brace),
68+
xml_nodes_to_lint,
69+
source_file = source_expression,
70+
lint_message = "There should be a space before an opening curly brace."
71+
))
72+
73+
xp_cond_closed <- xp_and(c(
74+
# matching { is on same line
75+
if (isTRUE(allow_single_line)) {
76+
"(@line1 != preceding-sibling::OP-LEFT-BRACE/@line1)"
77+
},
78+
# immediately followed by ",", "]" or ")"
79+
"not(
80+
@line1 = ancestor::expr/following-sibling::*[1][
81+
self::OP-COMMA or self::OP-RIGHT-BRACKET or self::OP-RIGHT-PAREN
82+
]/@line1
83+
)",
84+
# double curly
85+
"not(
86+
(@line1 = parent::expr/following-sibling::OP-RIGHT-BRACE/@line1) or
87+
(@line1 = preceding-sibling::expr/OP-RIGHT-BRACE/@line1)
88+
)"
89+
))
90+
91+
# TODO (AshesITR): if c_style_braces is TRUE, skip the not(ELSE) condition
92+
xp_closed_curly <- glue::glue("//OP-RIGHT-BRACE[
93+
{ xp_cond_closed } and (
94+
(@line1 = preceding-sibling::*[1]/@line2) or
95+
(@line1 = parent::expr/following-sibling::*[1][not(self::ELSE)]/@line1)
96+
)
97+
]")
98+
99+
lints <- c(lints, lapply(
100+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_closed_curly),
101+
xml_nodes_to_lint,
102+
source_file = source_expression,
103+
lint_message = paste(
104+
"Closing curly-braces should always be on their own line,",
105+
"unless they are followed by an else."
106+
)
107+
))
108+
109+
xp_else_closed_curly <- "preceding-sibling::IF/following-sibling::expr[2]/OP-RIGHT-BRACE"
110+
# need to (?) repeat previous_curly_path since != will return true if there is
111+
# no such node. ditto for approach with not(@line1 = ...).
112+
# TODO (AshesITR): if c_style_braces is TRUE, this needs to be @line2 + 1
113+
xp_else_same_line <- glue::glue("//ELSE[{xp_else_closed_curly} and @line1 != {xp_else_closed_curly}/@line2]")
114+
115+
lints <- c(lints, lapply(
116+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_else_same_line),
117+
xml_nodes_to_lint,
118+
source_file = source_expression,
119+
lint_message = "`else` should come on the same line as the previous `}`."
120+
))
121+
122+
xp_function_brace <- "//expr[FUNCTION and @line1 != @line2 and not(expr[OP-LEFT-BRACE])]"
123+
124+
lints <- c(lints, lapply(
125+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_function_brace),
126+
xml_nodes_to_lint,
127+
source_file = source_expression,
128+
lint_message = "Any function spanning multiple lines should use curly braces."
129+
))
130+
131+
# if (x) { ... } else if (y) { ... } else { ... } is OK; fully exact pairing
132+
# of if/else would require this to be
133+
# if (x) { ... } else { if (y) { ... } else { ... } } since there's no
134+
# elif operator/token in R, which is pretty unseemly
135+
xp_if_else_match_brace <- "
136+
//IF[
137+
following-sibling::expr[2][OP-LEFT-BRACE]
138+
and following-sibling::ELSE
139+
/following-sibling::expr[1][not(OP-LEFT-BRACE or IF/following-sibling::expr[2][OP-LEFT-BRACE])]
140+
]
141+
142+
|
143+
144+
//ELSE[
145+
following-sibling::expr[1][OP-LEFT-BRACE]
146+
and preceding-sibling::IF/following-sibling::expr[2][not(OP-LEFT-BRACE)]
147+
]
148+
"
149+
150+
lints <- c(lints, lapply(
151+
xml2::xml_find_all(source_expression$xml_parsed_content, xp_if_else_match_brace),
152+
xml_nodes_to_lint,
153+
source_file = source_expression,
154+
lint_message = "Either both or neither branch in `if`/`else` should use curly braces."
155+
))
156+
157+
lints
158+
})
159+
}

R/closed_curly_linter.R

+3-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#' <https://style.tidyverse.org/syntax.html#indenting>
1010
#' @export
1111
closed_curly_linter <- function(allow_single_line = FALSE) {
12+
lintr_deprecated("closed_curly_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
1213
Linter(function(source_file) {
1314
lapply(ids_with_token(source_file, "'}'"),
1415
function(id) {
@@ -66,7 +67,8 @@ closed_curly_linter <- function(allow_single_line = FALSE) {
6667
"unless they are followed by an else."
6768
),
6869
line = source_file$lines[as.character(parsed$line1)]
69-
)}
70+
)
71+
}
7072
}
7173
)
7274
})

R/else_same_line_linter.R

-31
This file was deleted.

R/function_brace_linter.R

-30
This file was deleted.

R/if_else_match_braces_linter.R

-48
This file was deleted.

R/open_curly_linter.R

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#' <https://style.tidyverse.org/syntax.html#indenting>
1010
#' @export
1111
open_curly_linter <- function(allow_single_line = FALSE) {
12+
lintr_deprecated("open_curly_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
1213
Linter(function(source_file) {
1314
lapply(
1415
ids_with_token(source_file, "'{'"),

R/paren_brace_linter.R

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#' @seealso [linters] for a complete list of linters available in lintr.
77
#' @export
88
paren_brace_linter <- function() {
9+
lintr_deprecated("paren_brace_linter", new = "brace_linter", version = "2.0.1.9001", type = "Linter")
910
Linter(function(source_file) {
1011
if (is.null(source_file$xml_parsed_content)) {
1112
return(NULL)

0 commit comments

Comments
 (0)