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

keyword_quote_linter for unnecessary quoting #2030

Merged
merged 28 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
bf390de
keyword_quote_linter for unnecessary quoting
MichaelChirico Aug 1, 2023
9b528d0
trim_some
MichaelChirico Aug 1, 2023
93e173e
import glue
MichaelChirico Aug 1, 2023
d118360
glue::glue -> glue
MichaelChirico Aug 1, 2023
29f5e8c
other glue functions
MichaelChirico Aug 1, 2023
7d49598
xml2:: replacement
MichaelChirico Aug 1, 2023
c7968de
more namespace importing
MichaelChirico Aug 1, 2023
67b5289
add examples
MichaelChirico Aug 2, 2023
92159c9
Merge branch 'main' into keyword_quote
MichaelChirico Aug 7, 2023
2c640a2
restore
MichaelChirico Aug 7, 2023
4f26866
line width
MichaelChirico Aug 7, 2023
2ca0de1
syntax in example
MichaelChirico Aug 7, 2023
8153028
remove explicit returns
MichaelChirico Aug 7, 2023
423a37e
use make.names()
MichaelChirico Aug 7, 2023
a25776e
reserved_words check not needed
MichaelChirico Aug 7, 2023
74866e9
unused constant
MichaelChirico Aug 7, 2023
18ab58e
split out two types of lints for extraction,assignment
MichaelChirico Aug 7, 2023
c76cf6c
small tweak
MichaelChirico Aug 7, 2023
b22457b
NEWS entry
MichaelChirico Aug 7, 2023
75afaf8
use xml_children, add tests, use shared names in tests
MichaelChirico Aug 8, 2023
ece4a4f
Merge branch 'main' into keyword_quote
MichaelChirico Aug 8, 2023
5e4e743
double quote
MichaelChirico Aug 8, 2023
fece979
xml_child
MichaelChirico Aug 8, 2023
b6bf4d1
Merge branch 'main' into keyword_quote
MichaelChirico Aug 8, 2023
aef3613
restore xml_children with comment
MichaelChirico Aug 8, 2023
39528e6
clarify comment
MichaelChirico Aug 8, 2023
522b02c
correct note about xml_child()
MichaelChirico Aug 8, 2023
df1114c
fix NEWS snippet
MichaelChirico Aug 8, 2023
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
Prev Previous commit
Next Next commit
more namespace importing
  • Loading branch information
MichaelChirico committed Aug 1, 2023
commit c7968de4566d8b7e4388a26a53e567fbf183a670
6 changes: 6 additions & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,10 @@ importFrom(utils,head)
importFrom(utils,relist)
importFrom(utils,tail)
importFrom(xml2,as_list)
importFrom(xml2,xml_attr)
importFrom(xml2,xml_attrs)
importFrom(xml2,xml_find_all)
importFrom(xml2,xml_find_chr)
importFrom(xml2,xml_find_first)
importFrom(xml2,xml_find_num)
importFrom(xml2,xml_text)
6 changes: 3 additions & 3 deletions R/T_and_F_symbol_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ T_and_F_symbol_linter <- function() { # nolint: object_name.
return(list())
}

bad_exprs <- xml2::xml_find_all(source_expression$xml_parsed_content, xpath)
bad_assigns <- xml2::xml_find_all(source_expression$xml_parsed_content, xpath_assignment)
bad_exprs <- xml_find_all(source_expression$xml_parsed_content, xpath)
bad_assigns <- xml_find_all(source_expression$xml_parsed_content, xpath_assignment)

make_lints <- function(expr, fmt) {
symbol <- xml2::xml_text(expr)
symbol <- xml_text(expr)
lint_message <- sprintf(fmt, replacement_map[symbol], symbol)
xml_nodes_to_lints(
xml = expr,
Expand Down
6 changes: 3 additions & 3 deletions R/any_duplicated_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,17 @@ any_duplicated_linter <- function() {

xml <- source_expression$xml_parsed_content

any_duplicated_expr <- xml2::xml_find_all(xml, any_duplicated_xpath)
any_duplicated_expr <- xml_find_all(xml, any_duplicated_xpath)
any_duplicated_lints <- xml_nodes_to_lints(
any_duplicated_expr,
source_expression = source_expression,
lint_message = "anyDuplicated(x, ...) > 0 is better than any(duplicated(x), ...).",
type = "warning"
)

length_unique_expr <- xml2::xml_find_all(xml, length_unique_xpath)
length_unique_expr <- xml_find_all(xml, length_unique_xpath)
lint_message <- ifelse(
is.na(xml2::xml_find_first(length_unique_expr, uses_nrow_xpath)),
is.na(xml_find_first(length_unique_expr, uses_nrow_xpath)),
"anyDuplicated(x) == 0L is better than length(unique(x)) == length(x).",
"anyDuplicated(DF$col) == 0L is better than length(unique(DF$col)) == nrow(DF)"
)
Expand Down
2 changes: 1 addition & 1 deletion R/any_is_na_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ any_is_na_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

xml_nodes_to_lints(
bad_expr,
Expand Down
8 changes: 4 additions & 4 deletions R/assignment_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -106,21 +106,21 @@ assignment_linter <- function(allow_cascading_assign = TRUE,

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
if (length(bad_expr) == 0L) {
return(list())
}

operator <- xml2::xml_text(bad_expr)
operator <- xml_text(bad_expr)
lint_message_fmt <- rep("Use <-, not %s, for assignment.", length(operator))
lint_message_fmt[operator %in% c("<<-", "->>")] <-
"%s can have hard-to-predict behavior; prefer assigning to a specific environment instead (with assign() or <-)."
lint_message_fmt[operator == "%<>%"] <-
"Avoid the assignment pipe %s; prefer using <- and %%>%% separately."

if (!allow_trailing) {
bad_trailing_expr <- xml2::xml_find_all(xml, trailing_assign_xpath)
trailing_assignments <- xml2::xml_attrs(bad_expr) %in% xml2::xml_attrs(bad_trailing_expr)
bad_trailing_expr <- xml_find_all(xml, trailing_assign_xpath)
trailing_assignments <- xml_attrs(bad_expr) %in% xml_attrs(bad_trailing_expr)
lint_message_fmt[trailing_assignments] <- "Assignment %s should not be trailing at the end of a line."
}

Expand Down
4 changes: 2 additions & 2 deletions R/backport_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ backport_linter <- function(r_version = getRversion(), except = character()) {

xml <- source_expression$xml_parsed_content

all_names_nodes <- xml2::xml_find_all(xml, names_xpath)
all_names <- xml2::xml_text(all_names_nodes)
all_names_nodes <- xml_find_all(xml, names_xpath)
all_names <- xml_text(all_names_nodes)

# not sapply/vapply, which may over-simplify to vector
# rbind makes sure we have a matrix with dimensions [n_versions x n_names]
Expand Down
2 changes: 1 addition & 1 deletion R/boolean_arithmetic_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ boolean_arithmetic_linter <- function() {

xml <- source_expression$xml_parsed_content

any_expr <- xml2::xml_find_all(xml, any_xpath)
any_expr <- xml_find_all(xml, any_xpath)

xml_nodes_to_lints(
any_expr,
Expand Down
12 changes: 6 additions & 6 deletions R/brace_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_open_curly),
xml_find_all(xml, xp_open_curly),
source_expression = source_expression,
lint_message =
"Opening curly braces should never go on their own line and should always be followed by a new line."
Expand All @@ -167,7 +167,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_paren_brace),
xml_find_all(xml, xp_paren_brace),
source_expression = source_expression,
lint_message = "There should be a space before an opening curly brace."
)
Expand All @@ -176,7 +176,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_closed_curly),
xml_find_all(xml, xp_closed_curly),
source_expression = source_expression,
lint_message =
"Closing curly-braces should always be on their own line, unless they are followed by an else."
Expand All @@ -186,7 +186,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_else_same_line),
xml_find_all(xml, xp_else_same_line),
source_expression = source_expression,
lint_message = "`else` should come on the same line as the previous `}`."
)
Expand All @@ -195,7 +195,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_function_brace),
xml_find_all(xml, xp_function_brace),
source_expression = source_expression,
lint_message = "Any function spanning multiple lines should use curly braces."
)
Expand All @@ -204,7 +204,7 @@ brace_linter <- function(allow_single_line = FALSE) {
lints <- c(
lints,
xml_nodes_to_lints(
xml2::xml_find_all(xml, xp_if_else_match_brace),
xml_find_all(xml, xp_if_else_match_brace),
source_expression = source_expression,
lint_message = "Either both or neither branch in `if`/`else` should use curly braces."
)
Expand Down
4 changes: 2 additions & 2 deletions R/class_equals_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ class_equals_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

operator <- xml_find_char(bad_expr, "string(*[2])")
operator <- xml_find_chr(bad_expr, "string(*[2])")
lint_message <- sprintf(
"Instead of comparing class(x) with %s, use inherits(x, 'class-name') or is.<class> or is(x, 'class')",
operator
Expand Down
4 changes: 2 additions & 2 deletions R/commas_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ commas_linter <- function() {
xml <- source_expression$xml_parsed_content

before_lints <- xml_nodes_to_lints(
xml2::xml_find_all(xml, xpath_before),
xml_find_all(xml, xpath_before),
source_expression = source_expression,
lint_message = "Commas should never have a space before.",
range_start_xpath = "number(./preceding-sibling::*[1]/@col2 + 1)", # start after preceding expression
range_end_xpath = "number(./@col1 - 1)" # end before comma
)

after_lints <- xml_nodes_to_lints(
xml2::xml_find_all(xml, xpath_after),
xml_find_all(xml, xpath_after),
source_expression = source_expression,
lint_message = "Commas should always have a space after.",
range_start_xpath = "number(./@col2 + 1)", # start and end after comma
Expand Down
4 changes: 2 additions & 2 deletions R/comment_linters.R
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ commented_code_linter <- function() {
if (!is_lint_level(source_expression, "file")) {
return(list())
}
all_comment_nodes <- xml2::xml_find_all(source_expression$full_xml_parsed_content, "//COMMENT")
all_comments <- xml2::xml_text(all_comment_nodes)
all_comment_nodes <- xml_find_all(source_expression$full_xml_parsed_content, "//COMMENT")
all_comments <- xml_text(all_comment_nodes)
code_candidates <- re_matches(all_comments, code_candidate_regex, global = FALSE, locations = TRUE)
extracted_code <- code_candidates[, "code"]
# ignore trailing ',' when testing for parsability
Expand Down
2 changes: 1 addition & 1 deletion R/condition_message_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ condition_message_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
sep_value <- get_r_string(bad_expr, xpath = "./expr/SYMBOL_SUB[text() = 'sep']/following-sibling::expr/STR_CONST")

bad_expr <- bad_expr[is.na(sep_value) | sep_value %in% c("", " ")]
Expand Down
4 changes: 2 additions & 2 deletions R/conjunct_test_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,14 @@ conjunct_test_linter <- function(allow_named_stopifnot = TRUE) {

xml <- source_expression$full_xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

if (length(bad_expr) == 0L) {
return(list())
}

matched_fun <- xp_call_name(bad_expr)
operator <- xml_find_char(bad_expr, "string(expr/*[self::AND2 or self::OR2])")
operator <- xml_find_chr(bad_expr, "string(expr/*[self::AND2 or self::OR2])")
replacement_fmt <- ifelse(
matched_fun %in% c("expect_true", "expect_false"),
"write multiple expectations like %1$s(A) and %1$s(B)",
Expand Down
2 changes: 1 addition & 1 deletion R/consecutive_assertion_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ consecutive_assertion_linter <- function() {

xml <- source_expression$full_xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

matched_function <- xp_call_name(bad_expr)
xml_nodes_to_lints(
Expand Down
2 changes: 1 addition & 1 deletion R/declared_functions.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ declared_s3_generics <- function(x) {
"/expr/SYMBOL/text()"
)

as.character(xml2::xml_find_all(x, xpath))
as.character(xml_find_all(x, xpath))
}
4 changes: 2 additions & 2 deletions R/duplicate_argument_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ duplicate_argument_linter <- function(except = c("mutate", "transmute")) {

xml <- source_expression$full_xml_parsed_content

calls <- xml2::xml_find_all(xml, xpath_call_with_args)
calls <- xml_find_all(xml, xpath_call_with_args)

if (length(except) > 0L) {
calls_text <- get_r_string(xp_call_name(calls))
calls <- calls[!(calls_text %in% except)]
}

all_arg_nodes <- lapply(calls, xml2::xml_find_all, xpath_arg_name)
all_arg_nodes <- lapply(calls, xml_find_all, xpath_arg_name)
arg_names <- lapply(all_arg_nodes, get_r_string)
is_duplicated <- lapply(arg_names, duplicated)

Expand Down
2 changes: 1 addition & 1 deletion R/empty_assignment_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ empty_assignment_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

xml_nodes_to_lints(
bad_expr,
Expand Down
2 changes: 1 addition & 1 deletion R/equals_na_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ equals_na_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

xml_nodes_to_lints(
bad_expr,
Expand Down
4 changes: 2 additions & 2 deletions R/expect_comparison_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ expect_comparison_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

comparator <- xml_find_char(bad_expr, "string(expr[2]/*[2])")
comparator <- xml_find_chr(bad_expr, "string(expr[2]/*[2])")
expectation <- comparator_expectation_map[comparator]
lint_message <- sprintf("%s(x, y) is better than expect_true(x %s y).", expectation, comparator)
xml_nodes_to_lints(bad_expr, source_expression, lint_message = lint_message, type = "warning")
Expand Down
2 changes: 1 addition & 1 deletion R/expect_identical_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ expect_identical_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
xml_nodes_to_lints(
bad_expr,
source_expression = source_expression,
Expand Down
2 changes: 1 addition & 1 deletion R/expect_length_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ expect_length_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
matched_function <- xp_call_name(bad_expr)
lint_message <- sprintf("expect_length(x, n) is better than %s(length(x), n)", matched_function)
xml_nodes_to_lints(bad_expr, source_expression, lint_message, type = "warning")
Expand Down
2 changes: 1 addition & 1 deletion R/expect_named_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ expect_named_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
matched_function <- xp_call_name(bad_expr)
lint_message <- sprintf("expect_named(x, n) is better than %s(names(x), n)", matched_function)

Expand Down
2 changes: 1 addition & 1 deletion R/expect_not_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ expect_not_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

xml_nodes_to_lints(
bad_expr,
Expand Down
2 changes: 1 addition & 1 deletion R/expect_null_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ expect_null_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

matched_function <- xp_call_name(bad_expr)
msg <- ifelse(
Expand Down
2 changes: 1 addition & 1 deletion R/expect_s3_class_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ expect_s3_class_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
matched_function <- xp_call_name(bad_expr)
msg <- ifelse(
matched_function %in% c("expect_equal", "expect_identical"),
Expand Down
2 changes: 1 addition & 1 deletion R/expect_s4_class_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ expect_s4_class_linter <- function() {
# TODO(michaelchirico): also catch expect_{equal,identical}(methods::is(x), k).
# this seems empirically rare, but didn't check many S4-heavy packages.

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
xml_nodes_to_lints(
bad_expr,
source_expression = source_expression,
Expand Down
4 changes: 2 additions & 2 deletions R/expect_true_false_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ expect_true_false_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)

# NB: use expr/$node, not expr[$node], to exclude other things (especially ns:: parts of the call)
call_name <- xp_call_name(bad_expr, condition = "starts-with(text(), 'expect_')")
truth_value <- xml_find_char(bad_expr, "string(expr/NUM_CONST[text() = 'TRUE' or text() = 'FALSE'])")
truth_value <- xml_find_chr(bad_expr, "string(expr/NUM_CONST[text() = 'TRUE' or text() = 'FALSE'])")
lint_message <- sprintf(
"expect_%s(x) is better than %s(x, %s)",
tolower(truth_value), call_name, truth_value
Expand Down
2 changes: 1 addition & 1 deletion R/expect_type_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ expect_type_linter <- function() {

xml <- source_expression$xml_parsed_content

bad_expr <- xml2::xml_find_all(xml, xpath)
bad_expr <- xml_find_all(xml, xpath)
matched_function <- xp_call_name(bad_expr)
msg <- ifelse(
matched_function %in% c("expect_equal", "expect_identical"),
Expand Down
4 changes: 2 additions & 2 deletions R/extraction_operator_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ extraction_operator_linter <- function() {
}

xml <- source_expression$xml_parsed_content
bad_exprs <- xml2::xml_find_all(xml, xpath)
msgs <- sprintf("Use `[[` instead of `%s` to extract an element.", xml2::xml_text(bad_exprs))
bad_exprs <- xml_find_all(xml, xpath)
msgs <- sprintf("Use `[[` instead of `%s` to extract an element.", xml_text(bad_exprs))

xml_nodes_to_lints(
bad_exprs,
Expand Down
4 changes: 2 additions & 2 deletions R/fixed_regex_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,12 @@ fixed_regex_linter <- function() {

xml <- source_expression$xml_parsed_content

patterns <- xml2::xml_find_all(xml, xpath)
patterns <- xml_find_all(xml, xpath)
pattern_strings <- get_r_string(patterns)
is_static <- is_not_regex(pattern_strings)

fixed_equivalent <- encodeString(get_fixed_string(pattern_strings[is_static]), quote = '"', justify = "none")
call_name <- xml_find_char(patterns[is_static], "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)")
call_name <- xml_find_chr(patterns[is_static], "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)")

is_stringr <- startsWith(call_name, "str_")
replacement <- ifelse(
Expand Down
Loading