-
Notifications
You must be signed in to change notification settings - Fork 186
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 assignment_as_infix = TRUE to indentation_linter() #1812
Changes from 11 commits
0d62332
ecd6dcd
94ee333
c2de2d0
9ad5ead
3635347
220c639
1dc0d29
75f5487
05ea7b6
59ec58d
e36addf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,27 @@ | |
#' # body | ||
#' } | ||
#' ``` | ||
#' @param assignment_as_infix Treat `<-` as a regular (i.e. left-associative) infix operator? | ||
#' This means, that infix operators on the right hand side of an assignment do not trigger a second level of | ||
#' indentation: | ||
#' ```r | ||
#' # complies to any style | ||
#' variable <- a %+% | ||
#' b %+% | ||
#' c | ||
#' | ||
#' # complies to assignment_as_infix = TRUE | ||
#' variable <- | ||
#' a %+% | ||
#' b %+% | ||
#' c | ||
#' | ||
#' # complies to assignment_as_infix = FALSE | ||
#' variable <- | ||
#' a %+% | ||
#' b %+% | ||
#' c | ||
#' ``` | ||
#' | ||
#' @examples | ||
#' # will produce lints | ||
|
@@ -97,7 +118,8 @@ | |
#' - <https://style.tidyverse.org/functions.html#long-lines-1> | ||
#' | ||
#' @export | ||
indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "always", "never")) { | ||
indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "always", "never"), | ||
assignment_as_infix = TRUE) { | ||
paren_tokens_left <- c("OP-LEFT-BRACE", "OP-LEFT-PAREN", "OP-LEFT-BRACKET", "LBB") | ||
paren_tokens_right <- c("OP-RIGHT-BRACE", "OP-RIGHT-PAREN", "OP-RIGHT-BRACKET", "OP-RIGHT-BRACKET") | ||
infix_tokens <- setdiff(infix_metadata$xml_tag, c("OP-LEFT-BRACE", "OP-COMMA", paren_tokens_left)) | ||
|
@@ -118,6 +140,24 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al | |
} | ||
} | ||
|
||
if (isTRUE(assignment_as_infix)) { | ||
suppressing_tokens <- c("LEFT_ASSIGN", "EQ_ASSIGN", "EQ_SUB", "EQ_FORMALS") | ||
xp_suppress <- glue::glue("preceding-sibling::{suppressing_tokens}[{xp_last_on_line}]") | ||
|
||
restoring_tokens <- c("expr[SYMBOL_FUNCTION_CALL]", "OP-LEFT-BRACE") | ||
xp_restore <- glue::glue("preceding-sibling::{restoring_tokens}") | ||
|
||
# match the first ancestor expr that is either | ||
# * a suppressing token (<- or =) or | ||
# * a restoring token (braces or a function call) | ||
# suppress the indent if the matched ancestor is a suppressing token | ||
infix_condition <- glue::glue(" | ||
and not(ancestor::expr[{xp_or(c(xp_suppress, xp_restore))}][1][{xp_or(xp_suppress)}]) | ||
") | ||
} else { | ||
infix_condition <- "" | ||
} | ||
|
||
xp_block_ends <- paste0( | ||
"number(", | ||
paste( | ||
|
@@ -141,7 +181,7 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al | |
@line2 > @line1 and | ||
({xp_or(paste0('descendant::', paren_tokens_left, '[', xp_last_on_line, ']'))}) | ||
]/@line1)]"), | ||
glue::glue("//{infix_tokens}[{xp_last_on_line}]"), | ||
glue::glue("//{infix_tokens}[{xp_last_on_line}{infix_condition}]"), | ||
glue::glue("//{no_paren_keywords}[{xp_last_on_line}]"), | ||
glue::glue("//{keyword_tokens}/following-sibling::OP-RIGHT-PAREN[ | ||
{xp_last_on_line} and | ||
|
@@ -186,17 +226,13 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al | |
change_end <- xml2::xml_find_num(change, xp_block_ends) | ||
if (isTRUE(change_begin <= change_end)) { | ||
to_indent <- seq(from = change_begin, to = change_end) | ||
if (change_type == "hanging") { | ||
expected_indent_levels[to_indent] <- as.integer(xml2::xml_attr(change, "col2")) | ||
is_hanging[to_indent] <- TRUE | ||
} else { # block or double | ||
if (change_type == "double") { | ||
expected_indent_levels[to_indent] <- expected_indent_levels[to_indent] + 2L * indent | ||
} else { | ||
expected_indent_levels[to_indent] <- expected_indent_levels[to_indent] + indent | ||
} | ||
is_hanging[to_indent] <- FALSE | ||
} | ||
expected_indent_levels[to_indent] <- find_new_indent( | ||
current_indent = expected_indent_levels[to_indent], | ||
change_type = change_type, | ||
indent = indent, | ||
hanging_indent = as.integer(xml2::xml_attr(change, "col2")) | ||
) | ||
is_hanging[to_indent] <- change_type == "hanging" | ||
} | ||
} | ||
|
||
|
@@ -261,6 +297,16 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al | |
}) | ||
} | ||
|
||
find_new_indent <- function(current_indent, change_type, indent, hanging_indent) { | ||
AshesITR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (change_type == "hanging") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe a |
||
hanging_indent | ||
} else if (change_type == "double") { | ||
current_indent + 2L * indent | ||
} else { | ||
current_indent + indent | ||
} | ||
} | ||
|
||
build_indentation_style_tidy <- function() { | ||
paren_tokens_left <- c("OP-LEFT-BRACE", "OP-LEFT-PAREN", "OP-LEFT-BRACKET", "LBB") | ||
paren_tokens_right <- c("OP-RIGHT-BRACE", "OP-RIGHT-PAREN", "OP-RIGHT-BRACKET", "OP-RIGHT-BRACKET") | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -634,6 +634,96 @@ test_that("hanging_indent_stlye works", { | |
expect_lint(code_hanging_same_line, "Indent", non_hanging_linter) | ||
}) | ||
|
||
test_that("assignment_as_infix works", { | ||
# test function call restorator and LEFT_ASSIGN suppressor | ||
code_infix <- trim_some(" | ||
ok_code <- | ||
var1 + | ||
f( | ||
var2 + | ||
var3 | ||
) + | ||
var4 | ||
") | ||
|
||
# test that innermost ancestor token decides the indentation | ||
code_infix_2 <- trim_some(" | ||
lapply(x, | ||
function(e) { | ||
temp_var <- | ||
e + | ||
42 | ||
} | ||
) | ||
") | ||
|
||
# test brace restorator | ||
code_infix_3 <- trim_some(" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's some horrible code 😄 |
||
ok_code <- | ||
if (condition) { | ||
a + | ||
b | ||
} else { | ||
c + | ||
d | ||
} + | ||
e | ||
") | ||
|
||
# test EQ_ASSIGN, EQ_SUB and EQ_FORMALS suppressors | ||
code_infix_4 <- trim_some(" | ||
# EQ_ASSIGN | ||
ok_code = | ||
a + | ||
b | ||
|
||
# EQ_SUB | ||
f( | ||
a = | ||
b + | ||
c | ||
) | ||
|
||
# EQ_FORMALS | ||
f <- function( | ||
a = | ||
b + | ||
c | ||
) { | ||
NULL | ||
} | ||
") | ||
|
||
code_no_infix <- trim_some(" | ||
ok_code <- | ||
var1 + | ||
f( | ||
var2 + | ||
var3 | ||
) + | ||
var4 | ||
") | ||
|
||
tidy_linter <- indentation_linter() | ||
no_infix_linter <- indentation_linter(assignment_as_infix = FALSE) | ||
|
||
expect_lint(code_infix, NULL, tidy_linter) | ||
expect_lint(code_infix_2, NULL, tidy_linter) | ||
expect_lint(code_infix_3, NULL, tidy_linter) | ||
expect_lint(code_infix_4, NULL, tidy_linter) | ||
expect_lint(code_no_infix, rex::rex("Indentation should be 2 spaces but is 4 spaces."), tidy_linter) | ||
|
||
expect_lint(code_infix, rex::rex("Indentation should be 4 spaces but is 2 spaces."), no_infix_linter) | ||
expect_lint(code_infix_2, rex::rex("Indentation should be 8 spaces but is 6 spaces."), no_infix_linter) | ||
expect_lint(code_infix_3, rex::rex("Indentation should be 4 spaces but is 2 spaces."), no_infix_linter) | ||
expect_lint(code_infix_4, list( | ||
list(line_number = 4L, rex::rex("Indentation should be 4 spaces but is 2 spaces.")), | ||
list(line_number = 10L, rex::rex("Indentation should be 6 spaces but is 4 spaces.")), | ||
list(line_number = 17L, rex::rex("Indentation should be 6 spaces but is 4 spaces.")) | ||
), no_infix_linter) | ||
expect_lint(code_no_infix, NULL, no_infix_linter) | ||
}) | ||
|
||
test_that("consecutive same-level lints are suppressed", { | ||
bad_code <- trim_some(" | ||
ok_code <- 42 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i usually prefer each argument with defaults on its own line once >1 line is used, WDYT? not sure it's a stated rule, but it's similar to the long lines rule for calls.