From dfa77849563a8b9c9d3b75f98f37ab0e18668ae3 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 1 Aug 2023 01:02:52 -0700 Subject: [PATCH 1/6] Use | more judiciously --- R/function_argument_linter.R | 2 +- R/indentation_linter.R | 8 +++++--- R/pipe_continuation_linter.R | 18 ++++++++---------- R/redundant_equals_linter.R | 11 ++++++----- R/vector_logic_linter.R | 7 +++---- 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/R/function_argument_linter.R b/R/function_argument_linter.R index 936a14fb0..dda0b8c9b 100644 --- a/R/function_argument_linter.R +++ b/R/function_argument_linter.R @@ -47,7 +47,7 @@ #' @export function_argument_linter <- function() { xpath <- paste(collapse = " | ", glue::glue(" - //{c('FUNCTION', 'OP-LAMBDA')} + (//FUNCTION | //OP-LAMBDA) /following-sibling::EQ_FORMALS[1] /following-sibling::SYMBOL_FORMALS[ text() != '...' diff --git a/R/indentation_linter.R b/R/indentation_linter.R index 9f98a6e2d..153d0998e 100644 --- a/R/indentation_linter.R +++ b/R/indentation_linter.R @@ -130,6 +130,8 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al hanging_indent_style <- match.arg(hanging_indent_style) + global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") + if (hanging_indent_style == "tidy") { find_indent_type <- build_indentation_style_tidy() } else if (hanging_indent_style == "always") { @@ -181,9 +183,9 @@ 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}{infix_condition}]"), - glue::glue("//{no_paren_keywords}[{xp_last_on_line}]"), - glue::glue("//{keyword_tokens}/following-sibling::OP-RIGHT-PAREN[ + glue::glue("({ global_nodes(infix_tokens) })[{xp_last_on_line}{infix_condition}]"), + glue::glue("({ global_nodes(no_paren_keywords) })[{xp_last_on_line}]"), + glue::glue("({ global_nodes(keyword_tokens) })/following-sibling::OP-RIGHT-PAREN[ {xp_last_on_line} and not(following-sibling::expr[1][OP-LEFT-BRACE]) ]") diff --git a/R/pipe_continuation_linter.R b/R/pipe_continuation_linter.R index 4580637d4..6f6463f5e 100644 --- a/R/pipe_continuation_linter.R +++ b/R/pipe_continuation_linter.R @@ -53,19 +53,17 @@ pipe_continuation_linter <- function() { # Where a single-line pipeline is nested inside a larger expression # e.g. inside a function definition), the outer expression can span multiple lines # without throwing a lint. - - pipe_conditions <- " + preceding_pipe <- "preceding-sibling::expr[1]/descendant::*[self::SPECIAL[text() = '%>%'] or self::PIPE]" + xpath <- glue::glue(" + (//PIPE | //SPECIAL[text() = '%>%'])[ parent::expr[@line1 < @line2] - and preceding-sibling::expr/descendant-or-self::*[self::SPECIAL[text() = '%>%'] or self::PIPE] + and {preceding_pipe} and ( - preceding-sibling::expr/descendant-or-self::expr/@line2 - = following-sibling::expr/descendant-or-self::expr/@line1 - or @line1 = preceding-sibling::expr/descendant-or-self::*[self::SPECIAL[text() = '%>%'] or self::PIPE]/@line1 + preceding-sibling::expr[1]/descendant-or-self::expr/@line2 + = following-sibling::expr[1]/descendant-or-self::expr/@line1 + or @line1 = {preceding_pipe}/@line1 ) - " - xpath <- glue::glue(" - //SPECIAL[text() = '%>%' and {pipe_conditions} ] - | //PIPE[ {pipe_conditions} ] + ] ") Linter(function(source_expression) { diff --git a/R/redundant_equals_linter.R b/R/redundant_equals_linter.R index d23811f17..274cde30a 100644 --- a/R/redundant_equals_linter.R +++ b/R/redundant_equals_linter.R @@ -36,11 +36,12 @@ #' - [outer_negation_linter()] #' @export redundant_equals_linter <- function() { - xpath <- paste0( - c("//EQ", "//NE"), - "/parent::expr/expr[NUM_CONST[text() = 'TRUE' or text() = 'FALSE']]/parent::expr", - collapse = " | " - ) + xpath <- " + (//EQ | //NE) + /parent::expr + /expr[NUM_CONST[text() = 'TRUE' or text() = 'FALSE']] + /parent::expr + " Linter(function(source_expression) { if (!is_lint_level(source_expression, "expression")) { diff --git a/R/vector_logic_linter.R b/R/vector_logic_linter.R index 388ed99b9..f50149d1a 100644 --- a/R/vector_logic_linter.R +++ b/R/vector_logic_linter.R @@ -60,8 +60,8 @@ vector_logic_linter <- function() { # ... # # we _don't_ want to match anything on the second expr, hence this - xpath_parts <- glue::glue(" - //{ c('AND', 'OR') }[ + xpath <- " + (//AND | //OR)[ ancestor::expr[ not(preceding-sibling::OP-RIGHT-PAREN) and preceding-sibling::*[ @@ -75,8 +75,7 @@ vector_logic_linter <- function() { or preceding-sibling::OP-LEFT-BRACKET ]) ] - ") - xpath <- paste(xpath_parts, collapse = " | ") + " Linter(function(source_expression) { if (!is_lint_level(source_expression, "expression")) { From ae0551bba472796cb9c824cf350eee1ef8ba8214 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 1 Aug 2023 17:08:47 +0000 Subject: [PATCH 2/6] feedback --- R/function_argument_linter.R | 4 ++-- R/indentation_linter.R | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/R/function_argument_linter.R b/R/function_argument_linter.R index dda0b8c9b..63cb5da32 100644 --- a/R/function_argument_linter.R +++ b/R/function_argument_linter.R @@ -46,14 +46,14 @@ #' - #' @export function_argument_linter <- function() { - xpath <- paste(collapse = " | ", glue::glue(" + xpath <- " (//FUNCTION | //OP-LAMBDA) /following-sibling::EQ_FORMALS[1] /following-sibling::SYMBOL_FORMALS[ text() != '...' and not(following-sibling::*[not(self::COMMENT)][1][self::EQ_FORMALS]) ] - ")) + " Linter(function(source_expression) { if (!is_lint_level(source_expression, "expression")) { diff --git a/R/indentation_linter.R b/R/indentation_linter.R index 153d0998e..7f6283934 100644 --- a/R/indentation_linter.R +++ b/R/indentation_linter.R @@ -130,8 +130,6 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al hanging_indent_style <- match.arg(hanging_indent_style) - global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") - if (hanging_indent_style == "tidy") { find_indent_type <- build_indentation_style_tidy() } else if (hanging_indent_style == "always") { @@ -177,6 +175,7 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al ")" ) + global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") # nolint: object_usage_linter. False positive. xp_indent_changes <- paste( c( glue::glue("//{paren_tokens_left}[not(@line1 = following-sibling::expr[ From 73cf585347d2bd96456083ca456a4d7c6ec851c1 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 1 Aug 2023 21:05:07 +0000 Subject: [PATCH 3/6] feedback #2 --- R/indentation_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/indentation_linter.R b/R/indentation_linter.R index 7f6283934..23a30cde4 100644 --- a/R/indentation_linter.R +++ b/R/indentation_linter.R @@ -175,7 +175,7 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al ")" ) - global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") # nolint: object_usage_linter. False positive. + global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") # nolint: object_usage. TODO(#2029): Fix this. xp_indent_changes <- paste( c( glue::glue("//{paren_tokens_left}[not(@line1 = following-sibling::expr[ From d4e93e167a950c82f1c82a0a4d247ad4d0d6bbce Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 2 Aug 2023 07:02:10 +0000 Subject: [PATCH 4/6] use in infix_spaces_linter() --- R/infix_spaces_linter.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/R/infix_spaces_linter.R b/R/infix_spaces_linter.R index 8dcdcc0fb..b8d2e5cca 100644 --- a/R/infix_spaces_linter.R +++ b/R/infix_spaces_linter.R @@ -147,7 +147,8 @@ infix_spaces_linter <- function(exclude_operators = NULL, allow_multiple_spaces # NB: parent::*[count(expr | SYMBOL_SUB)) > 1] for the unary case, e.g. x[-1] # SYMBOL_SUB for case with missing argument like alist(a =) # NB: the last not() disables lints inside box::use() declarations - xpath <- paste(collapse = "|", glue::glue("//{infix_tokens}[ + global_xpath <- paste0('//', infix_tokens, collapse = '|') + xpath <- glue::glue("({global_xpath})[ parent::*[count(expr | SYMBOL_SUB) > 1] and ( ( @@ -166,7 +167,7 @@ infix_spaces_linter <- function(exclude_operators = NULL, allow_multiple_spaces ] ] ) - ]")) + ]") Linter(function(source_expression) { if (!is_lint_level(source_expression, "expression")) { From c8191c21e6435ae7a60d728724d01410cecd798f Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 2 Aug 2023 07:11:06 +0000 Subject: [PATCH 5/6] quotes --- R/infix_spaces_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/infix_spaces_linter.R b/R/infix_spaces_linter.R index b8d2e5cca..2569a2f56 100644 --- a/R/infix_spaces_linter.R +++ b/R/infix_spaces_linter.R @@ -147,7 +147,7 @@ infix_spaces_linter <- function(exclude_operators = NULL, allow_multiple_spaces # NB: parent::*[count(expr | SYMBOL_SUB)) > 1] for the unary case, e.g. x[-1] # SYMBOL_SUB for case with missing argument like alist(a =) # NB: the last not() disables lints inside box::use() declarations - global_xpath <- paste0('//', infix_tokens, collapse = '|') + global_xpath <- paste0("//", infix_tokens, collapse = "|") xpath <- glue::glue("({global_xpath})[ parent::*[count(expr | SYMBOL_SUB) > 1] and ( From 5463bf1d75d05cbfe34726316a4ef273a5a85048 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 2 Aug 2023 10:21:44 -0700 Subject: [PATCH 6/6] remove false positive nolint --- R/indentation_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/indentation_linter.R b/R/indentation_linter.R index 23a30cde4..5d369f4ab 100644 --- a/R/indentation_linter.R +++ b/R/indentation_linter.R @@ -175,7 +175,7 @@ indentation_linter <- function(indent = 2L, hanging_indent_style = c("tidy", "al ")" ) - global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") # nolint: object_usage. TODO(#2029): Fix this. + global_nodes <- function(nodes) paste0("//", nodes, collapse = "|") xp_indent_changes <- paste( c( glue::glue("//{paren_tokens_left}[not(@line1 = following-sibling::expr[