From b6889047a6fc90f18f70053cd5917b291cdd2e29 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 11:43:46 +0800 Subject: [PATCH 01/30] Batch of message consistency checks --- R/assignment_linter.R | 2 +- R/backport_linter.R | 5 +---- R/brace_linter.R | 2 +- R/class_equals_linter.R | 2 +- R/commas_linter.R | 4 ++-- R/comment_linters.R | 4 ++-- R/condition_call_linter.R | 41 ++++++++++------------------------- R/conjunct_test_linter.R | 6 ++--- R/cyclocomp_linter.R | 4 ++-- R/duplicate_argument_linter.R | 2 +- R/fixed_regex_linter.R | 15 ++++++++----- R/if_not_else_linter.R | 5 +---- R/implicit_integer_linter.R | 3 ++- R/is_numeric_linter.R | 4 ++-- 14 files changed, 40 insertions(+), 59 deletions(-) diff --git a/R/assignment_linter.R b/R/assignment_linter.R index 6bbe83d418..1043fa363b 100644 --- a/R/assignment_linter.R +++ b/R/assignment_linter.R @@ -111,7 +111,7 @@ assignment_linter <- function(allow_cascading_assign = TRUE, 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 <-)." + "Avoid %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." lint_message_fmt[operator == "%<>%"] <- "Avoid the assignment pipe %s; prefer using <- and %%>%% separately." diff --git a/R/backport_linter.R b/R/backport_linter.R index 22230bd1b6..7a37d80ff8 100644 --- a/R/backport_linter.R +++ b/R/backport_linter.R @@ -58,10 +58,7 @@ backport_linter <- function(r_version = getRversion(), except = character()) { needs_backport <- !is.na(bad_versions) lint_message <- sprintf( - paste( - "%s (R %s) is not available for dependency R >= %s.", - "Use the `except` argument of `backport_linter()` to configure available backports." - ), + "%s (R %s) is not available for dependency R >= %s.", all_names[needs_backport], bad_versions[needs_backport], r_version diff --git a/R/brace_linter.R b/R/brace_linter.R index 1c0337a24c..417be06845 100644 --- a/R/brace_linter.R +++ b/R/brace_linter.R @@ -194,7 +194,7 @@ brace_linter <- function(allow_single_line = FALSE) { xml_nodes_to_lints( xml_find_all(xml, xp_function_brace), source_expression = source_expression, - lint_message = "Any function spanning multiple lines should use curly braces." + lint_message = "Use curly braces for any function spanning multiple lines." ) ) diff --git a/R/class_equals_linter.R b/R/class_equals_linter.R index 5ee712cf1a..0571fd8793 100644 --- a/R/class_equals_linter.R +++ b/R/class_equals_linter.R @@ -52,7 +52,7 @@ class_equals_linter <- function() { 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. or is(x, 'class')", + "use inherits(x, 'class-name') or is. or is(x, 'class') instead of comparing class(x) with %s.", operator ) xml_nodes_to_lints( diff --git a/R/commas_linter.R b/R/commas_linter.R index e7b9feda76..407278bd7d 100644 --- a/R/commas_linter.R +++ b/R/commas_linter.R @@ -84,7 +84,7 @@ commas_linter <- function(allow_trailing = FALSE) { before_lints <- xml_nodes_to_lints( xml_find_all(xml, xpath_before), source_expression = source_expression, - lint_message = "Commas should never have a space before.", + lint_message = "Commas should never follow a space.", range_start_xpath = "number(./preceding-sibling::*[1]/@col2 + 1)", # start after preceding expression range_end_xpath = "number(./@col1 - 1)" # end before comma ) @@ -92,7 +92,7 @@ commas_linter <- function(allow_trailing = FALSE) { after_lints <- xml_nodes_to_lints( xml_find_all(xml, xpath_after), source_expression = source_expression, - lint_message = "Commas should always have a space after.", + lint_message = "Commas should always precede a space.", range_start_xpath = "number(./@col2 + 1)", # start and end after comma range_end_xpath = "number(./@col2 + 1)" ) diff --git a/R/comment_linters.R b/R/comment_linters.R index 26c107ffc2..e444a9b32c 100644 --- a/R/comment_linters.R +++ b/R/comment_linters.R @@ -91,7 +91,7 @@ commented_code_linter <- function() { lint_list <- xml_nodes_to_lints( all_comment_nodes[is_parsable], source_expression = source_expression, - lint_message = "Commented code should be removed." + lint_message = "Remove commented code." ) # Location info needs updating @@ -175,7 +175,7 @@ todo_comment_linter <- function(todo = c("todo", "fixme")) { line_number = token[["line1"]], column_number = token[["col1"]], type = "style", - message = "TODO comments should be removed.", + message = "Remove TODO comments.", line = source_expression[["lines"]][[as.character(token[["line1"]])]], ranges = list(c(token[["col1"]], token[["col2"]])) ) diff --git a/R/condition_call_linter.R b/R/condition_call_linter.R index 1a618d8baa..b30ff24ae9 100644 --- a/R/condition_call_linter.R +++ b/R/condition_call_linter.R @@ -58,58 +58,41 @@ condition_call_linter <- function(display_call = FALSE) { call_xpath <- glue::glue(" following-sibling::SYMBOL_SUB[text() = 'call.'] - /following-sibling::expr[1] - /NUM_CONST[text() = '{!display_call}'] + /following-sibling::expr[1] + /NUM_CONST[text() = '{!display_call}'] ") - no_call_xpath <- " - parent::expr[ - count(SYMBOL_SUB[text() = 'call.']) = 0 - ] - " + no_call_xpath <- "parent::expr[not(SYMBOL_SUB[text() = 'call.'])]" if (is.na(display_call)) { - frag <- no_call_xpath + call_cond <- no_call_xpath + msg_fmt <- "Provide an explicit value for `call.` in %s()." } else if (display_call) { - frag <- call_xpath + call_cond <- call_xpath + msg_fmt <- "Use %s(.) to display the call in an error message." } else { # call. = TRUE can be expressed in two way: # - either explicitly with call. = TRUE # - or by implicitly relying on the default - frag <- xp_or(call_xpath, no_call_xpath) + call_cond <- xp_or(call_xpath, no_call_xpath) + msg_fmt <- "Use %s(., call. = FALSE) not to display the call in an error message." } xpath <- glue::glue(" //SYMBOL_FUNCTION_CALL[text() = 'stop' or text() = 'warning'] - /parent::expr[{frag}] - /parent::expr + /parent::expr[{call_cond}] + /parent::expr ") Linter(linter_level = "expression", function(source_expression) { - xml <- source_expression$xml_parsed_content if (is.null(xml)) return(list()) bad_expr <- xml_find_all(xml, xpath) - if (is.na(display_call)) { - msg <- glue::glue( - "Provide an explicit value for call. in {xp_call_name(bad_expr)}()." - ) - } else if (display_call) { - msg <- glue::glue( - "Use {xp_call_name(bad_expr)}(.) to display call in error message." - ) - } else { - msg <- glue::glue( - "Use {xp_call_name(bad_expr)}(., call. = FALSE)", - " to not display call in error message." - ) - } - xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = msg, + lint_message = sprintf(msg_fmt, xp_call_name(bad_expr)), type = "warning" ) }) diff --git a/R/conjunct_test_linter.R b/R/conjunct_test_linter.R index 92f3d51a80..b85804adcf 100644 --- a/R/conjunct_test_linter.R +++ b/R/conjunct_test_linter.R @@ -127,13 +127,13 @@ conjunct_test_linter <- function(allow_named_stopifnot = TRUE, operator <- xml_find_chr(test_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)", - "write multiple conditions like %s(A, B)." + "Write multiple expectations like %1$s(A) and %1$s(B)", + "Write multiple conditions like %s(A, B)" ) lint_message <- paste( - sprintf("Instead of %s(A %s B),", matched_fun, operator), # as.character() needed for 0-lint case where ifelse(logical(0)) returns logical(0) sprintf(as.character(replacement_fmt), matched_fun), + sprintf("instead of %s(A %s B).", matched_fun, operator), "The latter will produce better error messages in the case of failure." ) lints <- xml_nodes_to_lints( diff --git a/R/cyclocomp_linter.R b/R/cyclocomp_linter.R index 7288ff07cf..c5563646f2 100644 --- a/R/cyclocomp_linter.R +++ b/R/cyclocomp_linter.R @@ -36,8 +36,8 @@ cyclocomp_linter <- function(complexity_limit = 15L) { column_number = source_expression[["column"]][1L], type = "style", message = sprintf( - "Functions should have cyclomatic complexity of less than %d, this has %d.", - complexity_limit, complexity + "Reduce the cyclomatic complexity of this function from %d to at most %d.", + complexity, complexity_limit ), ranges = list(rep(col1, 2L)), line = source_expression$lines[1L] diff --git a/R/duplicate_argument_linter.R b/R/duplicate_argument_linter.R index edcb673abe..883d16faeb 100644 --- a/R/duplicate_argument_linter.R +++ b/R/duplicate_argument_linter.R @@ -58,7 +58,7 @@ duplicate_argument_linter <- function(except = c("mutate", "transmute")) { xml_nodes_to_lints( unlist(all_arg_nodes, recursive = FALSE)[unlist(is_duplicated)], source_expression = source_expression, - lint_message = "Duplicate arguments in function call.", + lint_message = "Remove duplicate arguments in function call.", type = "warning" ) }) diff --git a/R/fixed_regex_linter.R b/R/fixed_regex_linter.R index d124a3a180..47beb39a53 100644 --- a/R/fixed_regex_linter.R +++ b/R/fixed_regex_linter.R @@ -145,10 +145,13 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) { patterns <- xml_find_all(xml, xpath) pattern_strings <- get_r_string(patterns) + is_static <- is_not_regex(pattern_strings, allow_unescaped) + patterns <- patterns[is_static] + pattern_strings <- pattern_strings[is_static] - fixed_equivalent <- encodeString(get_fixed_string(pattern_strings[is_static]), quote = '"', justify = "none") - call_name <- xml_find_chr(patterns[is_static], "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)") + fixed_equivalent <- encodeString(get_fixed_string(pattern_strings), quote = '"', justify = "none") + call_name <- xml_find_chr(patterns, "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)") is_stringr <- startsWith(call_name, "str_") replacement <- ifelse( @@ -157,13 +160,13 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) { fixed_equivalent ) msg <- paste( - "This regular expression is static, i.e., its matches can be expressed as a fixed substring expression, which", - "is faster to compute. Here, you can use", - replacement, ifelse(is_stringr, "as the pattern.", "with fixed = TRUE.") + "Use", replacement, ifelse(is_stringr, "as the pattern", "with fixed = TRUE"), "here.", + "This regular expression is static, i.e., its matches can be expressed as a fixed substring expression, ", + "which is faster to compute." ) xml_nodes_to_lints( - patterns[is_static], + patterns, source_expression = source_expression, lint_message = msg, type = "warning" diff --git a/R/if_not_else_linter.R b/R/if_not_else_linter.R index 731ca4625c..58ffa214ba 100644 --- a/R/if_not_else_linter.R +++ b/R/if_not_else_linter.R @@ -91,10 +91,7 @@ if_not_else_linter <- function(exceptions = c("is.null", "is.na", "missing")) { if_lints <- xml_nodes_to_lints( if_expr, source_expression = source_expression, - lint_message = paste( - "In a simple if/else statement,", - "prefer `if (A) x else y` to the less-readable `if (!A) y else x`." - ), + lint_message = "Prefer `if (A) x else y` to the less-readable `if (!A) y else x` in a simple if/else statement.", type = "warning" ) diff --git a/R/implicit_integer_linter.R b/R/implicit_integer_linter.R index 354120e6a3..ea3a4bbebc 100644 --- a/R/implicit_integer_linter.R +++ b/R/implicit_integer_linter.R @@ -60,7 +60,8 @@ implicit_integer_linter <- function(allow_colon = FALSE) { xml_nodes_to_lints( numbers[is_implicit_integer(xml_text(numbers))], source_expression = source_expression, - lint_message = "Integers should not be implicit. Use the form 1L for integers or 1.0 for doubles.", + lint_message = + "Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.", type = "style", column_number_xpath = "number(./@col2 + 1)", # mark at end range_end_xpath = "number(./@col2 + 1)" # end after number for easy fixing (enter "L" or ".0") diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R index e75ba54729..e8737c3ca6 100644 --- a/R/is_numeric_linter.R +++ b/R/is_numeric_linter.R @@ -78,7 +78,7 @@ is_numeric_linter <- function() { or_expr, source_expression = source_expression, lint_message = paste( - "is.numeric(x) is the same as is.numeric(x) || is.integer(x).", + "Use is.numeric(x) instead of equivalent is.numeric(x) || is.integer(x).", "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" @@ -97,7 +97,7 @@ is_numeric_linter <- function() { class_expr, source_expression = source_expression, lint_message = paste( - 'is.numeric(x) is the same as class(x) %in% c("integer", "numeric").', + 'Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric").', "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" From 413d87b6a27e5041b5e79be4c2c59e3d5edd04da Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 3 Dec 2023 23:11:18 -0800 Subject: [PATCH 02/30] Update R/class_equals_linter.R Co-authored-by: Indrajeet Patil --- R/class_equals_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/class_equals_linter.R b/R/class_equals_linter.R index 0571fd8793..89c419522c 100644 --- a/R/class_equals_linter.R +++ b/R/class_equals_linter.R @@ -52,7 +52,7 @@ class_equals_linter <- function() { operator <- xml_find_chr(bad_expr, "string(*[2])") lint_message <- sprintf( - "use inherits(x, 'class-name') or is. or is(x, 'class') instead of comparing class(x) with %s.", + "Use inherits(x, 'class-name') or is. or is(x, 'class') instead of comparing class(x) with %s.", operator ) xml_nodes_to_lints( From 910c158fe9487fbc874f2e46a219e0bcdacf1160 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 3 Dec 2023 23:19:07 -0800 Subject: [PATCH 03/30] Update R/is_numeric_linter.R Co-authored-by: Indrajeet Patil --- R/is_numeric_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R index e8737c3ca6..05ba934c80 100644 --- a/R/is_numeric_linter.R +++ b/R/is_numeric_linter.R @@ -78,7 +78,7 @@ is_numeric_linter <- function() { or_expr, source_expression = source_expression, lint_message = paste( - "Use is.numeric(x) instead of equivalent is.numeric(x) || is.integer(x).", + "Use `is.numeric(x)` instead of equivalent `is.numeric(x) || is.integer(x)`.", "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" From 915a656bc0a29a91b5ab3ab14279be00ad2ebceb Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 15:45:51 +0800 Subject: [PATCH 04/30] finish first pass --- R/assignment_linter.R | 2 +- R/namespace_linter.R | 2 +- R/nested_ifelse_linter.R | 4 ++-- R/nested_pipe_linter.R | 2 +- R/nzchar_linter.R | 4 ++-- R/one_call_pipe_linter.R | 2 +- R/paren_body_linter.R | 2 +- R/pipe_consistency_linter.R | 3 +-- R/pipe_continuation_linter.R | 2 +- R/pipe_return_linter.R | 5 ++--- R/semicolon_linter.R | 4 ++-- R/seq_linter.R | 8 ++++---- R/stopifnot_all_linter.R | 4 ++-- R/strings_as_factors_linter.R | 8 ++------ R/terminal_close_linter.R | 5 +---- R/trailing_blank_lines_linter.R | 4 ++-- R/trailing_whitespace_linter.R | 2 +- R/undesirable_function_linter.R | 2 +- R/undesirable_operator_linter.R | 2 +- R/unnecessary_concatenation_linter.R | 13 +++++-------- R/unreachable_code_linter.R | 8 ++++---- R/unused_import_linter.R | 2 +- R/vector_logic_linter.R | 2 +- R/yoda_test_linter.R | 2 +- 24 files changed, 41 insertions(+), 53 deletions(-) diff --git a/R/assignment_linter.R b/R/assignment_linter.R index 1043fa363b..ce2698119e 100644 --- a/R/assignment_linter.R +++ b/R/assignment_linter.R @@ -111,7 +111,7 @@ assignment_linter <- function(allow_cascading_assign = TRUE, operator <- xml_text(bad_expr) lint_message_fmt <- rep("Use <-, not %s, for assignment.", length(operator)) lint_message_fmt[operator %in% c("<<-", "->>")] <- - "Avoid %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." + "Replace %s by assigning to a specific environment (with assign() or <-) to avoid hard-to-predict behavior." lint_message_fmt[operator == "%<>%"] <- "Avoid the assignment pipe %s; prefer using <- and %%>%% separately." diff --git a/R/namespace_linter.R b/R/namespace_linter.R index e0bc85eb5c..7dd32ffffe 100644 --- a/R/namespace_linter.R +++ b/R/namespace_linter.R @@ -157,7 +157,7 @@ build_ns_get_int_lints <- function(packages, symbols, symbol_nodes, namespaces, symbol_nodes[exported], source_expression = source_expression, lint_message = - sprintf("'%1$s' is exported from {%2$s}. Use %2$s::%1$s instead.", symbols[exported], packages[exported]), + sprintf("Use %2$s::%1$s, not %2$s::%1$s.", symbols[exported], packages[exported]), type = "warning" ) diff --git a/R/nested_ifelse_linter.R b/R/nested_ifelse_linter.R index 8657333ff9..0ce604e075 100644 --- a/R/nested_ifelse_linter.R +++ b/R/nested_ifelse_linter.R @@ -94,8 +94,8 @@ nested_ifelse_linter <- function() { matched_call <- xp_call_name(bad_expr) lint_message <- paste( - sprintf("Don't use nested %s() calls;", matched_call), - "instead, try (1) data.table::fcase; (2) dplyr::case_when; or (3) using a lookup table." + sprintf("Avoid nested %s() calls.", matched_call), + "Instead, try (1) data.table::fcase; (2) dplyr::case_when; or (3) using a lookup table." ) xml_nodes_to_lints(bad_expr, source_expression, lint_message, type = "warning") }) diff --git a/R/nested_pipe_linter.R b/R/nested_pipe_linter.R index a7a8c323b7..46190e0805 100644 --- a/R/nested_pipe_linter.R +++ b/R/nested_pipe_linter.R @@ -76,7 +76,7 @@ nested_pipe_linter <- function( xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = "Don't nest pipes inside other calls.", + lint_message = "Avoid nesting pipes inside other calls.", type = "warning" ) }) diff --git a/R/nzchar_linter.R b/R/nzchar_linter.R index b9e36b89f1..f15b4d88f0 100644 --- a/R/nzchar_linter.R +++ b/R/nzchar_linter.R @@ -101,7 +101,7 @@ nzchar_linter <- function() { comparison_expr, source_expression = source_expression, lint_message = paste( - 'Instead of comparing strings to "", use nzchar().', + 'Use nzchar() instead of comparing strings to "".', "Note that if x is a factor, you'll have use ", 'as.character() to replicate an implicit conversion that happens in x == "".', keepna_note @@ -114,7 +114,7 @@ nzchar_linter <- function() { nchar_expr, source_expression = source_expression, lint_message = paste( - "Instead of comparing nchar(x) to 0, use nzchar().", + "Use nzchar() instead of comparing nchar(x) to 0.", keepna_note ), type = "warning" diff --git a/R/one_call_pipe_linter.R b/R/one_call_pipe_linter.R index f780ce02b9..ab27e51721 100644 --- a/R/one_call_pipe_linter.R +++ b/R/one_call_pipe_linter.R @@ -75,7 +75,7 @@ one_call_pipe_linter <- function() { xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = paste0("Expressions with only a single call shouldn't use pipe ", pipe, "."), + lint_message = paste0("Avoid pipe ", pipe, " for expressions with only a single call."), type = "warning" ) }) diff --git a/R/paren_body_linter.R b/R/paren_body_linter.R index eb44a80872..b8ec09e407 100644 --- a/R/paren_body_linter.R +++ b/R/paren_body_linter.R @@ -47,6 +47,6 @@ paren_body_linter <- make_linter_from_xpath( ] /following-sibling::expr ", - lint_message = "There should be a space between a right parenthesis and a body expression.", + lint_message = "Put a space between a right parenthesis and a body expression.", type = "style" ) diff --git a/R/pipe_consistency_linter.R b/R/pipe_consistency_linter.R index d463f584ad..85b429aa42 100644 --- a/R/pipe_consistency_linter.R +++ b/R/pipe_consistency_linter.R @@ -53,8 +53,7 @@ pipe_consistency_linter <- function(pipe = c("auto", "%>%", "|>")) { xml = c(match_magrittr, match_native), source_expression = source_expression, lint_message = glue( - "Found {n_magrittr} instances of %>% and {n_native} instances of |>. ", - "Stick to one pipe operator." + "Stick to one pipe operator; found {n_magrittr} instances of %>% and {n_native} instances of |>." ), type = "style" ) diff --git a/R/pipe_continuation_linter.R b/R/pipe_continuation_linter.R index 0c03cf46c6..aabe58f77b 100644 --- a/R/pipe_continuation_linter.R +++ b/R/pipe_continuation_linter.R @@ -78,7 +78,7 @@ pipe_continuation_linter <- function() { pipe_exprs, source_expression = source_expression, lint_message = sprintf( - "`%s` should always have a space before it and a new line after it, unless the full pipeline fits on one line.", + "Put a space before `%s` and a new line after it, unless the full pipeline fits on one line.", pipe_text ), type = "style" diff --git a/R/pipe_return_linter.R b/R/pipe_return_linter.R index fd73da8ae1..57b5bcafe6 100644 --- a/R/pipe_return_linter.R +++ b/R/pipe_return_linter.R @@ -32,8 +32,7 @@ pipe_return_linter <- make_linter_from_xpath( /following-sibling::expr[expr/SYMBOL_FUNCTION_CALL[text() = 'return']] ", lint_message = paste( - "Using return() as the final step of a magrittr pipeline", - "is an anti-pattern. Instead, assign the output of the pipeline to", - "a well-named object and return that." + "Avoid return() as the final step of a magrittr pipeline. ", + "Instead, assign the output of the pipeline to a well-named object and return that." ) ) diff --git a/R/semicolon_linter.R b/R/semicolon_linter.R index 66b893e6d1..4feca5b95b 100644 --- a/R/semicolon_linter.R +++ b/R/semicolon_linter.R @@ -60,8 +60,8 @@ #' - #' @export semicolon_linter <- function(allow_compound = FALSE, allow_trailing = FALSE) { - msg_trailing <- "Trailing semicolons are not needed." - msg_compound <- "Compound semicolons are discouraged. Replace them by a newline." + msg_trailing <- "Remove trailing semicolons." + msg_compound <- "Replace compound semicolons by a newline." if (allow_compound && allow_trailing) { stop( diff --git a/R/seq_linter.R b/R/seq_linter.R index 462c0889af..a2e08523cd 100644 --- a/R/seq_linter.R +++ b/R/seq_linter.R @@ -104,12 +104,12 @@ seq_linter <- function() { lint_message <- ifelse( grepl("seq", dot_expr1, fixed = TRUE), sprintf( - "%s(%s) is likely to be wrong in the empty edge case. Use %s instead.", - dot_expr1, dot_expr2, replacement + "Use %s instead of %s(%s), which is likely to be wrong in the empty edge case.", + replacement, dot_expr1, dot_expr2 ), sprintf( - "%s:%s is likely to be wrong in the empty edge case. Use %s instead.", - dot_expr1, dot_expr2, replacement + "Use %s instead of %s:%s, which is likely to be wrong in the empty edge case.", + replacement, dot_expr1, dot_expr2 ) ) diff --git a/R/stopifnot_all_linter.R b/R/stopifnot_all_linter.R index f081cc0c05..7ec01ea3ee 100644 --- a/R/stopifnot_all_linter.R +++ b/R/stopifnot_all_linter.R @@ -37,7 +37,7 @@ stopifnot_all_linter <- make_linter_from_xpath( /expr[expr/SYMBOL_FUNCTION_CALL[text() = 'all']] ", lint_message = paste( - "Calling stopifnot(all(x)) is redundant. stopifnot(x) runs all()", - "'under the hood' and provides a better error message in case of failure." + "Use stopifnot(x) instead of stopifnot(all(x)).", + "stopifnot(x) runs all() 'under the hood' and provides a better error message in case of failure." ) ) diff --git a/R/strings_as_factors_linter.R b/R/strings_as_factors_linter.R index e4cf535fdc..97522182ce 100644 --- a/R/strings_as_factors_linter.R +++ b/R/strings_as_factors_linter.R @@ -91,12 +91,8 @@ strings_as_factors_linter <- function() { xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = paste( - "This code relies on the default value of stringsAsFactors,", - "which changed in version R 4.0. Please supply an explicit value for", - "stringsAsFactors for this code to work with versions of R both before", - "and after this switch." - ), + lint_message = + "Supply an explicit value for stringsAsFactors for this code to work before and after R version 4.0.", type = "warning" ) }) diff --git a/R/terminal_close_linter.R b/R/terminal_close_linter.R index 4b8a3ede32..20b86ac645 100644 --- a/R/terminal_close_linter.R +++ b/R/terminal_close_linter.R @@ -49,8 +49,5 @@ terminal_close_linter <- make_linter_from_xpath( ] ] ", - lint_message = paste( - "Use on.exit(close(x)) to close connections instead of", - "running it as the last call in a function." - ) + lint_message = "Use on.exit(close(x)) to close connections instead of running it as the last call in a function." ) diff --git a/R/trailing_blank_lines_linter.R b/R/trailing_blank_lines_linter.R index 3479a8b586..101d778aa3 100644 --- a/R/trailing_blank_lines_linter.R +++ b/R/trailing_blank_lines_linter.R @@ -38,7 +38,7 @@ trailing_blank_lines_linter <- function() { line_number = line_number, column_number = 1L, type = "style", - message = "Trailing blank lines are superfluous.", + message = "Remove trailing blank lines.", line = source_expression$file_lines[[line_number]] ) } @@ -53,7 +53,7 @@ trailing_blank_lines_linter <- function() { line_number = length(source_expression$file_lines), column_number = (nchar(last_line) %||% 0L) + 1L, type = "style", - message = "Missing terminal newline.", + message = "Add a terminal newline.", line = last_line ) } diff --git a/R/trailing_whitespace_linter.R b/R/trailing_whitespace_linter.R index fec39dcba5..ba1e5ef257 100644 --- a/R/trailing_whitespace_linter.R +++ b/R/trailing_whitespace_linter.R @@ -71,7 +71,7 @@ trailing_whitespace_linter <- function(allow_empty_lines = FALSE, allow_in_strin line_number = line, column_number = res$start[[line]], type = "style", - message = "Trailing whitespace is superfluous.", + message = "Remove trailing whitespace.", line = source_expression$file_lines[[line]], ranges = list(c(res$start[[line]], res$end[[line]])) ) diff --git a/R/undesirable_function_linter.R b/R/undesirable_function_linter.R index d157b39630..3b4f225bc8 100644 --- a/R/undesirable_function_linter.R +++ b/R/undesirable_function_linter.R @@ -91,7 +91,7 @@ undesirable_function_linter <- function(fun = default_undesirable_functions, msgs <- vapply( stats::setNames(nm = unique(fun_names)), function(fun_name) { - msg <- sprintf('Function "%s" is undesirable.', fun_name) + msg <- sprintf('Avoid undesirable function "%s".', fun_name) alternative <- fun[[fun_name]] if (!is.na(alternative)) { msg <- paste(msg, sprintf("As an alternative, %s.", alternative)) diff --git a/R/undesirable_operator_linter.R b/R/undesirable_operator_linter.R index e25ab27f18..4831491193 100644 --- a/R/undesirable_operator_linter.R +++ b/R/undesirable_operator_linter.R @@ -73,7 +73,7 @@ undesirable_operator_linter <- function(op = default_undesirable_operators) { bad_op <- xml_find_all(xml, xpath) operator <- xml_text(bad_op) - lint_message <- sprintf("Operator `%s` is undesirable.", operator) + lint_message <- sprintf("Avoid undesirable operator `%s`.", operator) alternative <- op[operator] has_alternative <- !is.na(alternative) lint_message[has_alternative] <- paste(lint_message[has_alternative], alternative[has_alternative]) diff --git a/R/unnecessary_concatenation_linter.R b/R/unnecessary_concatenation_linter.R index 3571e5fafb..d33e28cecb 100644 --- a/R/unnecessary_concatenation_linter.R +++ b/R/unnecessary_concatenation_linter.R @@ -57,13 +57,10 @@ unnecessary_concatenation_linter <- function(allow_single_expression = TRUE) { # length(allow_single_expression) == 1L ) - msg_empty <- paste( - "Unneeded concatenation without arguments.", - 'Replace the "c" call by NULL or, whenever possible,', - "vector() seeded with the correct type and/or length." - ) + msg_empty <- + "Replace unnecessary c() by NULL or, whenever possible, vector() seeded with the correct type and/or length." - msg_const <- 'Unneeded concatenation of a constant. Remove the "c" call.' + msg_const <- 'Remove unnecessary c() of a constant.' non_constant_cond <- "SYMBOL or (expr and not(OP-COLON and count(expr[SYMBOL or expr]) != 2))" @@ -85,8 +82,8 @@ unnecessary_concatenation_linter <- function(allow_single_expression = TRUE) { # path_to_non_constant <- glue("./expr[2][ {non_constant_cond} ]") msg_const_expr <- paste( - 'Unneeded concatenation of a simple expression. Remove the "c" call,', - 'replacing with "as.vector" if using "c" to string attributes, e.g. in converting an array to a vector.' + "Remove unnecessary c() of a constant expression.", + "Replace with as.vector() if c() is used to strip attributes, e.g. in converting an array to a vector." ) } call_xpath <- glue(" diff --git a/R/unreachable_code_linter.R b/R/unreachable_code_linter.R index f84aab614c..9ce5900d6d 100644 --- a/R/unreachable_code_linter.R +++ b/R/unreachable_code_linter.R @@ -148,7 +148,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud lints_return_stop <- xml_nodes_to_lints( drop_valid_comments(expr_return_stop, allow_comment_regex), source_expression = source_expression, - lint_message = "Code and comments coming after a return() or stop() should be removed.", + lint_message = "Remove code and comments coming after return() or stop().", type = "warning" ) @@ -157,7 +157,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud lints_next_break <- xml_nodes_to_lints( drop_valid_comments(expr_next_break, allow_comment_regex), source_expression = source_expression, - lint_message = "Code and comments coming after a `next` or `break` should be removed.", + lint_message = "Remove code and comments coming after `next` or `break`.", type = "warning" ) @@ -166,7 +166,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud lints_if_while <- xml_nodes_to_lints( expr_if_while, source_expression = source_expression, - lint_message = "Code inside a conditional loop with a deterministically false condition should be removed.", + lint_message = "Remove code inside a conditional loop with a deterministically false condition.", type = "warning" ) @@ -175,7 +175,7 @@ unreachable_code_linter <- function(allow_comment_regex = getOption("covr.exclud lints_else <- xml_nodes_to_lints( expr_else, source_expression = source_expression, - lint_message = "Code inside an else block after a deterministically true if condition should be removed.", + lint_message = "Remove code inside an else block after a deterministically true condition.", type = "warning" ) diff --git a/R/unused_import_linter.R b/R/unused_import_linter.R index 707e856266..504fb2bcc1 100644 --- a/R/unused_import_linter.R +++ b/R/unused_import_linter.R @@ -129,7 +129,7 @@ unused_import_linter <- function(allow_ns_usage = FALSE, lint_message <- ifelse( is_ns_used[is_unused][unused_packages], paste0( - "Package '", unused_packages, "' is only used by namespace. ", + "Don't attach package '", unused_packages, "', which is only used by namespace. ", "Check that it is installed using loadNamespace() instead." ), paste0("Package '", unused_packages, "' is attached but never used.") diff --git a/R/vector_logic_linter.R b/R/vector_logic_linter.R index bcc3e995c3..c0c961b9c4 100644 --- a/R/vector_logic_linter.R +++ b/R/vector_logic_linter.R @@ -89,7 +89,7 @@ vector_logic_linter <- function() { xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = "Conditional expressions require scalar logical operators (&& and ||)", + lint_message = "Use scalar logical operators (&& and ||) in conditional expressions.", type = "warning" ) }) diff --git a/R/yoda_test_linter.R b/R/yoda_test_linter.R index 442a77f640..bff26a46e9 100644 --- a/R/yoda_test_linter.R +++ b/R/yoda_test_linter.R @@ -66,7 +66,7 @@ yoda_test_linter <- function() { lint_message <- ifelse( is.na(second_const), paste( - "Tests should compare objects in the order 'actual', 'expected', not the reverse.", + "Compare objects in tests in the order 'actual', 'expected', not the reverse.", sprintf("For example, do %1$s(foo(x), 2L) instead of %1$s(2L, foo(x)).", matched_call) ), sprintf("Avoid storing placeholder tests like %s(1, 1)", matched_call) From b7f2c44b0c368f33db3d8a9f07862c325f2dfe86 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 08:33:44 +0000 Subject: [PATCH 05/30] progress fixing tests --- tests/testthat/test-seq_linter.R | 32 +++++------ .../testthat/test-strings_as_factors_linter.R | 8 +-- tests/testthat/test-todo_comment_linter.R | 2 +- .../test-trailing_blank_lines_linter.R | 18 +++--- .../test-trailing_whitespace_linter.R | 18 +++--- .../test-undesirable_function_linter.R | 4 +- .../test-undesirable_operator_linter.R | 16 +++--- .../test-unnecessary_concatenation_linter.R | 57 ++++++++----------- tests/testthat/test-unreachable_code_linter.R | 30 +++++----- tests/testthat/test-unused_import_linter.R | 4 +- tests/testthat/test-vector_logic_linter.R | 6 +- tests/testthat/test-yoda_test_linter.R | 41 ++++++------- 12 files changed, 109 insertions(+), 127 deletions(-) diff --git a/tests/testthat/test-seq_linter.R b/tests/testthat/test-seq_linter.R index f7a3446e97..d04e40e158 100644 --- a/tests/testthat/test-seq_linter.R +++ b/tests/testthat/test-seq_linter.R @@ -66,37 +66,37 @@ test_that("finds 1:length(...) expressions", { expect_lint( "function(x) { 1:NROW(x) }", - rex::rex("NROW(...)", anything, "Use seq_len"), + rex::rex("Use seq_len(NROW(...)) instead of 1:NROW(...)"), linter ) expect_lint( "function(x) { 1:NCOL(x) }", - rex::rex("NCOL(...)", anything, "Use seq_len"), + rex::rex("Use seq_len(NCOL(...)) instead of 1:NCOL(...)"), linter ) expect_lint( "function(x) { 1:dim(x)[1L] }", - rex::rex("dim(...)", anything, "Use seq_len"), + rex::rex("Use seq_len(dim(...)[1L]) instead of 1:dim(...)[1L]"), linter ) expect_lint( "function(x) { 1L:dim(x)[[1]] }", - rex::rex("dim(...)", anything, "Use seq_len"), + rex::rex("Use seq_len", anything, "dim(...)"), linter ) expect_lint( "function(x) { mutate(x, .id = 1:n()) }", - rex::rex("n() is", anything, "Use seq_len"), + rex::rex("Use seq_len(n()) instead of 1:n(),"), linter ) expect_lint( "function(x) { x[, .id := 1:.N] }", - rex::rex(".N is", anything, "Use seq_len"), + rex::rex("Use seq_len(.N) instead of 1:.N,"), linter ) }) @@ -104,7 +104,7 @@ test_that("finds 1:length(...) expressions", { test_that("1L is also bad", { expect_lint( "function(x) { 1L:length(x) }", - rex::rex("1L:length(...)", anything, "Use seq_along"), + rex::rex("seq_along", anything, "1L:length(...)"), seq_linter() ) }) @@ -116,7 +116,7 @@ test_that("reverse seq is ok", { expect_lint( "function(x) { length(x):1 }", - rex::rex("length(...):1", anything, "Use rev(seq_along(...))"), + rex::rex("rev(seq_along(...))", anything, "length(...):1"), seq_linter() ) }) @@ -125,8 +125,8 @@ test_that("Message vectorization works for multiple lints", { expect_lint( "c(1:length(x), 1:nrow(y))", list( - rex::rex("1:length(...)", anything, "seq_along(...)"), - rex::rex("1:nrow(...)", anything, "seq_len(nrow(...))") + rex::rex("seq_along(...)", anything, "1:length(...)"), + rex::rex("seq_len(nrow(...))", anything, "1:nrow(...)") ), seq_linter() ) @@ -134,8 +134,8 @@ test_that("Message vectorization works for multiple lints", { expect_lint( "c(seq(length(x)), 1:nrow(y))", list( - rex::rex("seq(length(...))", anything, "seq_along(...)"), - rex::rex("1:nrow(...)", anything, "seq_len(nrow(...))") + rex::rex("seq_along(...)", anything, "seq(length(...))"), + rex::rex("seq_len(nrow(...))", anything, "1:nrow(...)") ), seq_linter() ) @@ -143,8 +143,8 @@ test_that("Message vectorization works for multiple lints", { expect_lint( "c(seq(length(x)), seq(nrow(y)))", list( - rex::rex("seq(length(...))", anything, "seq_along(...)"), - rex::rex("seq(nrow(...))", anything, "seq_len(nrow(...))") + rex::rex("seq_along(...)", anything, "seq(length(...))"), + rex::rex("seq_len(nrow(...))", anything, "seq(nrow(...))") ), seq_linter() ) @@ -152,8 +152,8 @@ test_that("Message vectorization works for multiple lints", { expect_lint( "c(1:NROW(x), seq(NCOL(y)))", list( - rex::rex("1:NROW(...)", anything, "seq_len(NROW(...)"), - rex::rex("seq(NCOL(...))", anything, "seq_len(NCOL(...))") + rex::rex("seq_len(NROW(...))", anything, "1:NROW(...)"), + rex::rex("seq_len(NCOL(...))", anything, "seq(NCOL(...))") ), seq_linter() ) diff --git a/tests/testthat/test-strings_as_factors_linter.R b/tests/testthat/test-strings_as_factors_linter.R index 725f987764..4c91a29104 100644 --- a/tests/testthat/test-strings_as_factors_linter.R +++ b/tests/testthat/test-strings_as_factors_linter.R @@ -21,7 +21,7 @@ test_that("strings_as_factors_linter skips allowed usages", { test_that("strings_as_factors_linter blocks simple disallowed usages", { linter <- strings_as_factors_linter() - lint_msg <- "This code relies on the default value of stringsAsFactors" + lint_msg <- "Supply an explicit value for stringsAsFactors for this code" expect_lint("data.frame('a')", lint_msg, linter) expect_lint("data.frame(c('a', 'b'))", lint_msg, linter) @@ -38,7 +38,7 @@ test_that("strings_as_factors_linter blocks simple disallowed usages", { test_that("strings_as_factors_linters catches rep(char) usages", { linter <- strings_as_factors_linter() - lint_msg <- "This code relies on the default value of stringsAsFactors" + lint_msg <- "Supply an explicit value for stringsAsFactors for this code" expect_lint("data.frame(rep('a', 10L))", lint_msg, linter) expect_lint("data.frame(rep(c('a', 'b'), 10L))", lint_msg, linter) @@ -52,7 +52,7 @@ test_that("strings_as_factors_linters catches rep(char) usages", { test_that("strings_as_factors_linter catches character(), as.character() usages", { linter <- strings_as_factors_linter() - lint_msg <- "This code relies on the default value of stringsAsFactors" + lint_msg <- "Supply an explicit value for stringsAsFactors for this code" expect_lint("data.frame(a = character())", lint_msg, linter) expect_lint("data.frame(a = character(1L))", lint_msg, linter) @@ -64,7 +64,7 @@ test_that("strings_as_factors_linter catches character(), as.character() usages" test_that("strings_as_factors_linter catches more functions with string output", { linter <- strings_as_factors_linter() - lint_msg <- "This code relies on the default value of stringsAsFactors" + lint_msg <- "Supply an explicit value for stringsAsFactors for this code" expect_lint("data.frame(a = paste(1, 2, 3))", lint_msg, linter) expect_lint("data.frame(a = sprintf('%d', 1:10))", lint_msg, linter) diff --git a/tests/testthat/test-todo_comment_linter.R b/tests/testthat/test-todo_comment_linter.R index 2d8abf520c..5a8c8e98f8 100644 --- a/tests/testthat/test-todo_comment_linter.R +++ b/tests/testthat/test-todo_comment_linter.R @@ -1,6 +1,6 @@ test_that("returns the correct linting", { linter <- todo_comment_linter(todo = c("todo", "fixme")) - lint_msg <- "TODO comments should be removed." + lint_msg <- rex::rex("Remove TODO comments.") expect_lint("a <- \"you#need#to#fixme\"", NULL, linter) expect_lint("# something todo", NULL, linter) diff --git a/tests/testthat/test-trailing_blank_lines_linter.R b/tests/testthat/test-trailing_blank_lines_linter.R index 7ec9229ca3..b5833e8a93 100644 --- a/tests/testthat/test-trailing_blank_lines_linter.R +++ b/tests/testthat/test-trailing_blank_lines_linter.R @@ -12,7 +12,7 @@ test_that("trailing_blank_lines_linter doesn't block allowed usages", { test_that("trailing_blank_lines_linter detects disallowed usages", { linter <- trailing_blank_lines_linter() - lint_msg <- rex::rex("Trailing blank lines are superfluous.") + lint_msg <- rex::rex("Remove trailing blank lines.") expect_lint("blah <- 1\n", lint_msg, linter) expect_lint("blah <- 1\n ", lint_msg, linter) @@ -27,7 +27,7 @@ test_that("trailing_blank_lines_linter detects disallowed usages", { expect_lint( file = tmp2, checks = list( - message = rex::rex("Missing terminal newline."), + message = rex::rex("Add a terminal newline."), line_number = 1L, column_number = 10L ), @@ -57,7 +57,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp3, checks = list( - message = rex::rex("Missing terminal newline."), + message = rex::rex("Add a terminal newline."), line_number = 10L, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. column_number = 1L @@ -80,7 +80,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp4, checks = list( - message = rex::rex("Missing terminal newline."), + message = rex::rex("Add a terminal newline."), line_number = 5L, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. column_number = 1L @@ -108,7 +108,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp5, checks = list( - message = rex::rex("Missing terminal newline."), + message = rex::rex("Add a terminal newline."), line_number = 10L, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. column_number = 1L @@ -137,7 +137,7 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp6, - checks = list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 7L, column_number = 1L), + checks = list(message = rex::rex("Remove trailing blank lines."), line_number = 7L, column_number = 1L), linters = linter ) @@ -161,9 +161,9 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp7, checks = list( - list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 7L, column_number = 1L), - list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 8L, column_number = 1L), - list(message = rex::rex("Trailing blank lines are superfluous."), line_number = 9L, column_number = 1L) + list(message = rex::rex("Remove trailing blank lines."), line_number = 7L, column_number = 1L), + list(message = rex::rex("Remove trailing blank lines."), line_number = 8L, column_number = 1L), + list(message = rex::rex("Remove trailing blank lines."), line_number = 9L, column_number = 1L) ), linters = linter ) diff --git a/tests/testthat/test-trailing_whitespace_linter.R b/tests/testthat/test-trailing_whitespace_linter.R index e9159d87a0..329f5a24fb 100644 --- a/tests/testthat/test-trailing_whitespace_linter.R +++ b/tests/testthat/test-trailing_whitespace_linter.R @@ -1,39 +1,37 @@ test_that("returns the correct linting", { linter <- trailing_whitespace_linter() + lint_msg <- rex::rex("Remove trailing whitespace.") expect_lint("blah", NULL, linter) expect_lint( "blah <- 1 ", - list(message = rex::rex("Trailing whitespace is superfluous."), column_number = 10L), + list(message = lint_msg, column_number = 10L), linter ) - expect_lint( - "blah <- 1 \n'hi'", - rex::rex("Trailing whitespace is superfluous."), - linter - ) + expect_lint("blah <- 1 \n'hi'", lint_msg, linter) expect_lint( "blah <- 1\n'hi'\na <- 2 ", - list(message = rex::rex("Trailing whitespace is superfluous."), line_number = 3L), + list(message = lint_msg, line_number = 3L), linter ) }) test_that("also handles completely empty lines per allow_empty_lines argument", { linter <- trailing_whitespace_linter() + lint_msg <- rex::rex("Remove trailing whitespace.") expect_lint( "blah <- 1\n \n'hi'\na <- 2", - list(message = rex::rex("Trailing whitespace is superfluous."), line_number = 2L), + list(message = lint_msg, line_number = 2L), linter ) expect_lint( "blah <- 1 ", - list(message = rex::rex("Trailing whitespace is superfluous."), column_number = 10L), + list(message = lint_msg, column_number = 10L), trailing_whitespace_linter(allow_empty_lines = TRUE) ) @@ -46,7 +44,7 @@ test_that("also handles completely empty lines per allow_empty_lines argument", test_that("also handles trailing whitespace in string constants", { linter <- trailing_whitespace_linter() - lint_msg <- rex::rex("Trailing whitespace is superfluous.") + lint_msg <- rex::rex("Remove trailing whitespace.") expect_lint("blah <- ' \n \n'", NULL, linter) # Don't exclude past the end of string diff --git a/tests/testthat/test-undesirable_function_linter.R b/tests/testthat/test-undesirable_function_linter.R index 6597904ea9..c12cbf21bf 100644 --- a/tests/testthat/test-undesirable_function_linter.R +++ b/tests/testthat/test-undesirable_function_linter.R @@ -1,7 +1,7 @@ test_that("linter returns correct linting", { linter <- undesirable_function_linter(fun = c(return = NA, log10 = "use log()")) - msg_return <- "Function \"return\" is undesirable.$" - msg_log10 <- "Function \"log10\" is undesirable. As an alternative, use log\\(\\)." + msg_return <- rex::rex('Avoid undesirable function "return".', end) + msg_log10 <- rex::rex('Avoid undesirable function "log10". As an alternative, use log().') expect_lint("x <- options()", NULL, linter) expect_lint("cat(\"Try to return\")", NULL, linter) diff --git a/tests/testthat/test-undesirable_operator_linter.R b/tests/testthat/test-undesirable_operator_linter.R index 9e98d40b11..35aa6c20b9 100644 --- a/tests/testthat/test-undesirable_operator_linter.R +++ b/tests/testthat/test-undesirable_operator_linter.R @@ -1,7 +1,7 @@ test_that("linter returns correct linting", { linter <- undesirable_operator_linter(op = c("$" = "As an alternative, use the `[[` accessor.", "<<-" = NA)) - msg_assign <- rex::escape("Operator `<<-` is undesirable.") - msg_dollar <- rex::escape("Operator `$` is undesirable. As an alternative, use the `[[` accessor.") + msg_assign <- rex::escape("Avoid undesirable operator `<<-`.") + msg_dollar <- rex::escape("Avoid undesirable operator `$`. As an alternative, use the `[[` accessor.") expect_lint("x <- foo:::getObj()", NULL, linter) expect_lint("cat(\"10$\")", NULL, linter) @@ -20,20 +20,20 @@ test_that("linter returns correct linting", { test_that("undesirable_operator_linter handles '=' consistently", { linter <- undesirable_operator_linter(op = c("=" = "As an alternative, use '<-'")) - expect_lint("a = 2L", rex::rex("Operator `=` is undesirable."), linter) + expect_lint("a = 2L", rex::rex("Avoid undesirable operator `=`."), linter) expect_lint("lm(data = mtcars)", NULL, linter) expect_lint("function(a = 1) { }", NULL, linter) }) test_that("undesirable_operator_linter handles infixes correctly", { linter <- undesirable_operator_linter(list("%oo%" = NA)) - expect_lint("a %oo% b", rex::rex("Operator `%oo%` is undesirable"), linter) + expect_lint("a %oo% b", rex::rex("Avoid undesirable operator `%oo%`."), linter) expect_lint("a %00% b", NULL, linter) # somewhat special case: %% is in infix_metadata expect_lint( "foo(x %% y, x %/% y)", - rex::rex("Operator `%%` is undesirable"), + rex::rex("Avoid undesirable operator `%%`."), undesirable_operator_linter(list("%%" = NA)) ) }) @@ -42,9 +42,9 @@ test_that("undesirable_operator_linter vectorizes messages", { expect_lint( "x <<- c(pkg:::foo, bar %oo% baz)", list( - rex::rex("`<<-` is undesirable. It assigns"), - rex::rex("`:::` is undesirable. It accesses"), - rex::rex("`%oo%` is undesirable.", end) + rex::rex("Avoid undesirable operator `<<-`. It assigns"), + rex::rex("Avoid undesirable operator `:::`. It accesses"), + rex::rex("Avoid undesirable operator `%oo%`.", end) ), undesirable_operator_linter(modify_defaults(default_undesirable_operators, "%oo%" = NA)) ) diff --git a/tests/testthat/test-unnecessary_concatenation_linter.R b/tests/testthat/test-unnecessary_concatenation_linter.R index ba1e10078c..e7af5f46be 100644 --- a/tests/testthat/test-unnecessary_concatenation_linter.R +++ b/tests/testthat/test-unnecessary_concatenation_linter.R @@ -12,8 +12,8 @@ test_that("unnecessary_concatenation_linter skips allowed usages", { test_that("unnecessary_concatenation_linter blocks disallowed usages", { linter <- unnecessary_concatenation_linter() - msg_c <- rex::escape('Unneeded concatenation of a constant. Remove the "c" call.') - msg_e <- rex::escape('Unneeded concatenation without arguments. Replace the "c" call by NULL') + msg_c <- rex::rex("Remove unnecessary c() of a constant.") + msg_e <- rex::rex("Replace unnecessary c() by NULL or, whenever possible, vector()") expect_lint( "c()", @@ -48,20 +48,15 @@ test_that("unnecessary_concatenation_linter blocks disallowed usages", { local({ pipes <- pipes(exclude = "%$%") linter <- unnecessary_concatenation_linter() + const_msg <- rex::rex("Remove unnecessary c() of a constant.") + no_arg_msg <- rex::rex("Replace unnecessary c() by NULL or, whenever possible, vector()") + patrick::with_parameters_test_that( "Correctly handles concatenation within magrittr pipes", { expect_lint(sprintf('"a" %s c("b")', pipe), NULL, linter) - expect_lint( - sprintf('"a" %s c()', pipe), - "Unneeded concatenation of a constant", - linter - ) - expect_lint( - sprintf('"a" %s list("b", c())', pipe), - "Unneeded concatenation without arguments", - linter - ) + expect_lint(sprintf('"a" %s c()', pipe), const_msg, linter) + expect_lint(sprintf('"a" %s list("b", c())', pipe), no_arg_msg, linter) }, pipe = pipes, .test_name = names(pipes) @@ -69,37 +64,35 @@ local({ }) test_that("symbolic expressions are allowed, except by request", { - expect_lint("c(alpha / 2)", NULL, unnecessary_concatenation_linter()) - expect_lint("c(paste0('.', 1:2))", NULL, unnecessary_concatenation_linter()) - expect_lint("c(DF[cond > 1, col])", NULL, unnecessary_concatenation_linter()) + linter <- unnecessary_concatenation_linter() + linter_strict <- unnecessary_concatenation_linter(allow_single_expression = FALSE) + message <- rex::rex("Remove unnecessary c() of a constant expression.") + + expect_lint("c(alpha / 2)", NULL, linter) + expect_lint("c(paste0('.', 1:2))", NULL, linter) + expect_lint("c(DF[cond > 1, col])", NULL, linter) # allow_single_expression = FALSE turns both into lints - linter <- unnecessary_concatenation_linter(allow_single_expression = FALSE) - message <- "Unneeded concatenation of a simple expression" - expect_lint("c(alpha / 2)", message, linter) - expect_lint("c(paste0('.', 1:2))", message, linter) - expect_lint("c(DF[cond > 1, col])", message, linter) + expect_lint("c(alpha / 2)", message, linter_strict) + expect_lint("c(paste0('.', 1:2))", message, linter_strict) + expect_lint("c(DF[cond > 1, col])", message, linter_strict) }) test_that("sequences with : are linted whenever a constant is involved", { linter <- unnecessary_concatenation_linter() - expect_lint("c(1:10)", "Unneeded concatenation of a constant", linter) - expect_lint("c(1:sum(x))", "Unneeded concatenation of a constant", linter) + linter_strict <- unnecessary_concatenation_linter(allow_single_expression = FALSE) + const_msg <- rex::rex("Remove unnecessary c() of a constant.") + expr_msg <- rex::rex("Remove unnecessary c() of a constant expression.") + + expect_lint("c(1:10)", const_msg, linter) + expect_lint("c(1:sum(x))", const_msg, linter) # this is slightly different if a,b are factors, in which case : does # something like interaction expect_lint("c(a:b)", NULL, linter) + expect_lint("c(a:b)", expr_msg, linter_strict) expect_lint("c(a:foo(b))", NULL, linter) - expect_lint( - "c(a:b)", - "Unneeded concatenation of a simple expression", - unnecessary_concatenation_linter(allow_single_expression = FALSE) - ) - expect_lint( - "c(a:foo(b))", - "Unneeded concatenation of a simple expression", - unnecessary_concatenation_linter(allow_single_expression = FALSE) - ) + expect_lint("c(a:foo(b))", expr_msg, linter_strict) }) test_that("c(...) does not lint under !allow_single_expression", { diff --git a/tests/testthat/test-unreachable_code_linter.R b/tests/testthat/test-unreachable_code_linter.R index 411ca146ec..4e3d58cb82 100644 --- a/tests/testthat/test-unreachable_code_linter.R +++ b/tests/testthat/test-unreachable_code_linter.R @@ -9,7 +9,7 @@ test_that("unreachable_code_linter works in simple function", { test_that("unreachable_code_linter works in sub expressions", { linter <- unreachable_code_linter() - msg <- rex::rex("Code and comments coming after a return() or stop()") + msg <- rex::rex("Remove code and comments coming after return() or stop()") lines <- trim_some(" foo <- function(bar) { @@ -106,7 +106,7 @@ test_that("unreachable_code_linter works in sub expressions", { test_that("unreachable_code_linter works with next and break in sub expressions", { linter <- unreachable_code_linter() - msg <- rex::rex("Code and comments coming after a `next` or `break`") + msg <- rex::rex("Remove code and comments coming after `next` or `break`") lines <- trim_some(" foo <- function(bar) { @@ -247,7 +247,7 @@ test_that("unreachable_code_linter identifies simple unreachable code", { lines, list( line_number = 3L, - message = rex::rex("Code and comments coming after a return() or stop()") + message = rex::rex("Remove code and comments coming after return() or stop()") ), unreachable_code_linter() ) @@ -263,13 +263,13 @@ test_that("unreachable_code_linter finds unreachable comments", { ") expect_lint( lines, - rex::rex("Code and comments coming after a return() or stop()"), + rex::rex("Remove code and comments coming after return() or stop()"), unreachable_code_linter() ) }) test_that("unreachable_code_linter finds expressions in the same line", { - msg <- rex::rex("Code and comments coming after a return() or stop()") + msg <- rex::rex("Remove code and comments coming after return() or stop()") linter <- unreachable_code_linter() lines <- trim_some(" @@ -297,7 +297,7 @@ test_that("unreachable_code_linter finds expressions in the same line", { }) test_that("unreachable_code_linter finds expressions and comments after comment in return line", { - msg <- rex::rex("Code and comments coming after a return() or stop()") + msg <- rex::rex("Remove code and comments coming after return() or stop()") linter <- unreachable_code_linter() lines <- trim_some(" @@ -326,7 +326,7 @@ test_that("unreachable_code_linter finds a double return", { ") expect_lint( lines, - rex::rex("Code and comments coming after a return() or stop()"), + rex::rex("Remove code and comments coming after return() or stop()"), unreachable_code_linter() ) }) @@ -341,7 +341,7 @@ test_that("unreachable_code_linter finds code after stop()", { ") expect_lint( lines, - rex::rex("Code and comments coming after a return() or stop()"), + rex::rex("Remove code and comments coming after return() or stop()"), unreachable_code_linter() ) }) @@ -412,7 +412,7 @@ test_that("unreachable_code_linter ignores terminal nolint end comments", { test_that("unreachable_code_linter identifies unreachable code in conditional loops", { linter <- unreachable_code_linter() - msg <- rex::rex("Code inside a conditional loop with a deterministically false condition should be removed.") + msg <- rex::rex("Remove code inside a conditional loop with a deterministically false condition.") lines <- trim_some(" foo <- function(bar) { @@ -493,7 +493,7 @@ test_that("unreachable_code_linter identifies unreachable code in conditional lo test_that("unreachable_code_linter identifies unreachable code in conditional loops", { linter <- unreachable_code_linter() - msg <- rex::rex("Code inside an else block after a deterministically true if condition should be removed.") + msg <- rex::rex("Remove code inside an else block after a deterministically true condition.") lines <- trim_some(" foo <- function(bar) { @@ -530,8 +530,8 @@ test_that("unreachable_code_linter identifies unreachable code in conditional lo test_that("unreachable_code_linter identifies unreachable code in mixed conditional loops", { linter <- unreachable_code_linter() - false_msg <- rex::rex("Code inside a conditional loop with a deterministically false condition should be removed.") - true_msg <- rex::rex("Code inside an else block after a deterministically true if condition should be removed.") + false_msg <- rex::rex("Remove code inside a conditional loop with a deterministically false condition.") + true_msg <- rex::rex("Remove code inside an else block after a deterministically true condition.") expect_lint( trim_some(" @@ -554,7 +554,7 @@ test_that("unreachable_code_linter identifies unreachable code in mixed conditio list(false_msg, line_number = 3L), list(false_msg, line_number = 6L), list(true_msg, line_number = 10L), - list(rex::rex("Code and comments coming after a return() or stop() should be removed."), line_number = 13L) + list(rex::rex("Remove code and comments coming after return() or stop()."), line_number = 13L) ), linter ) @@ -564,7 +564,7 @@ test_that("unreachable_code_linter identifies unreachable code in mixed conditio list( list(false_msg, line_number = 1L, ranges = list(c(12L, 17L))), list( - rex::rex("Code inside an else block after a deterministically true if condition should be removed."), + rex::rex("Remove code inside an else block after a deterministically true condition."), line_number = 1L, ranges = list(c(45L, 49L)) ) @@ -585,7 +585,7 @@ test_that("function shorthand is handled", { "), list( line_number = 3L, - message = rex::rex("Code and comments coming after a return() or stop()") + message = rex::rex("Remove code and comments coming after return() or stop()") ), unreachable_code_linter() ) diff --git a/tests/testthat/test-unused_import_linter.R b/tests/testthat/test-unused_import_linter.R index bb48a2ccac..4db47fe81c 100644 --- a/tests/testthat/test-unused_import_linter.R +++ b/tests/testthat/test-unused_import_linter.R @@ -18,7 +18,7 @@ test_that("unused_import_linter lints as expected", { expect_lint("library(dplyr, character.only = TRUE)\n1 + 1", NULL, linter) lint_msg <- rex::rex("Package 'dplyr' is attached but never used") - msg_ns <- rex::rex("Package 'dplyr' is only used by namespace") + msg_ns <- rex::rex("Don't attach package 'dplyr', which is only used by namespace.") expect_lint("library(dplyr)\n1 + 1", lint_msg, linter) expect_lint("require(dplyr)\n1 + 1", lint_msg, linter) @@ -43,7 +43,7 @@ test_that("unused_import_linter handles message vectorization", { "), list( rex::rex("Package 'crayon' is attached but never used."), - rex::rex("Package 'xmlparsedata' is only used by namespace") + rex::rex("Don't attach package 'xmlparsedata', which is only used by namespace") ), unused_import_linter() ) diff --git a/tests/testthat/test-vector_logic_linter.R b/tests/testthat/test-vector_logic_linter.R index 1b8c82295a..01bc79aef6 100644 --- a/tests/testthat/test-vector_logic_linter.R +++ b/tests/testthat/test-vector_logic_linter.R @@ -29,7 +29,7 @@ test_that("vector_logic_linter skips allowed usages", { test_that("vector_logic_linter blocks simple disallowed usages", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Conditional expressions require scalar logical operators") + lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") expect_lint("if (TRUE & FALSE) 1", lint_msg, linter) expect_lint("while (TRUE | TRUE) 2", lint_msg, linter) @@ -37,7 +37,7 @@ test_that("vector_logic_linter blocks simple disallowed usages", { test_that("vector_logic_linter detects nested conditions", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Conditional expressions require scalar logical operators") + lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") expect_lint("if (TRUE & TRUE || FALSE) 4", lint_msg, linter) expect_lint("if (TRUE && (TRUE | FALSE)) 4", lint_msg, linter) @@ -45,7 +45,7 @@ test_that("vector_logic_linter detects nested conditions", { test_that("vector_logic_linter catches usages in expect_true()/expect_false()", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Conditional expressions require scalar logical operators") + lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") expect_lint("expect_true(TRUE & FALSE)", lint_msg, linter) expect_lint("expect_false(TRUE | TRUE)", lint_msg, linter) diff --git a/tests/testthat/test-yoda_test_linter.R b/tests/testthat/test-yoda_test_linter.R index 2cd48fc033..a49b966d50 100644 --- a/tests/testthat/test-yoda_test_linter.R +++ b/tests/testthat/test-yoda_test_linter.R @@ -1,40 +1,31 @@ test_that("yoda_test_linter skips allowed usages", { - expect_lint("expect_equal(x, 2)", NULL, yoda_test_linter()) + linter <- yoda_test_linter() + + expect_lint("expect_equal(x, 2)", NULL, linter) # namespace qualification doesn't matter - expect_lint("testthat::expect_identical(x, 'a')", NULL, yoda_test_linter()) + expect_lint("testthat::expect_identical(x, 'a')", NULL, linter) # two variables can't be distinguished which is expected/actual (without # playing quixotic games trying to parse that out from variable names) - expect_lint("expect_equal(x, y)", NULL, yoda_test_linter()) + expect_lint("expect_equal(x, y)", NULL, linter) }) test_that("yoda_test_linter blocks simple disallowed usages", { - expect_lint( - "expect_equal(2, x)", - rex::rex("Tests should compare objects in the order 'actual', 'expected'"), - yoda_test_linter() - ) - expect_lint( - "testthat::expect_identical('a', x)", - rex::rex("Tests should compare objects in the order 'actual', 'expected'"), - yoda_test_linter() - ) - expect_lint( - "expect_setequal(2, x)", - rex::rex("Tests should compare objects in the order 'actual', 'expected'"), - yoda_test_linter() - ) + linter <- yoda_test_linter() + lint_msg <- rex::rex("Compare objects in tests in the order 'actual', 'expected', not the reverse.") + + expect_lint("expect_equal(2, x)", lint_msg, linter) + expect_lint("testthat::expect_identical('a', x)", lint_msg, linter) + expect_lint("expect_setequal(2, x)", lint_msg, linter) # complex literals are slightly odd - expect_lint( - "expect_equal(2 + 1i, x)", - rex::rex("Tests should compare objects in the order 'actual', 'expected'"), - yoda_test_linter() - ) + expect_lint("expect_equal(2 + 1i, x)", lint_msg, linter) }) test_that("yoda_test_linter ignores strings in $ expressions", { + linter <- yoda_test_linter() + # the "key" here shows up at the same level of the parse tree as plain "key" normally would - expect_lint('expect_equal(x$"key", 2)', NULL, yoda_test_linter()) - expect_lint('expect_equal(x@"key", 2)', NULL, yoda_test_linter()) + expect_lint('expect_equal(x$"key", 2)', NULL, linter) + expect_lint('expect_equal(x@"key", 2)', NULL, linter) }) # if we only inspect the first argument & ignore context, get false positives From a11a306582d84ae8714c11a37e732304d7f9b0e6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 10:15:55 +0000 Subject: [PATCH 06/30] a near-complete pass --- tests/testthat/test-commas_linter.R | 8 +++---- tests/testthat/test-commented_code_linter.R | 6 ++--- tests/testthat/test-condition_call_linter.R | 4 ++-- tests/testthat/test-conjunct_test_linter.R | 20 ++++++---------- tests/testthat/test-cyclocomp_linter.R | 4 ++-- .../testthat/test-duplicate_argument_linter.R | 6 ++--- tests/testthat/test-fixed_regex_linter.R | 12 +++++----- tests/testthat/test-if_not_else_linter.R | 6 ++--- tests/testthat/test-implicit_integer_linter.R | 6 ++--- tests/testthat/test-is_numeric_linter.R | 24 +++++++++++-------- tests/testthat/test-knitr_formats.R | 4 ++-- tests/testthat/test-nested_ifelse_linter.R | 14 +++++------ tests/testthat/test-nested_pipe_linter.R | 8 +++---- tests/testthat/test-nzchar_linter.R | 14 +++++------ tests/testthat/test-one_call_pipe_linter.R | 14 +++++------ tests/testthat/test-paren_body_linter.R | 23 ++++++++++-------- tests/testthat/test-pipe-consistency-linter.R | 10 ++++---- .../testthat/test-pipe_continuation_linter.R | 10 ++++---- tests/testthat/test-semicolon_linter.R | 4 ++-- tests/testthat/test-seq_linter.R | 14 +++++------ 20 files changed, 106 insertions(+), 105 deletions(-) diff --git a/tests/testthat/test-commas_linter.R b/tests/testthat/test-commas_linter.R index eaceb651f3..05e853a2fa 100644 --- a/tests/testthat/test-commas_linter.R +++ b/tests/testthat/test-commas_linter.R @@ -1,7 +1,7 @@ test_that("returns the correct linting (with default parameters)", { linter <- commas_linter() - msg_after <- rex::rex("Commas should always have a space after.") - msg_before <- rex::rex("Commas should never have a space before.") + msg_after <- rex::rex("Commas should always precede a space.") + msg_before <- rex::rex("Commas should never follow a space.") expect_lint("blah", NULL, linter) expect_lint("fun(1, 1)", NULL, linter) @@ -52,8 +52,8 @@ test_that("returns the correct linting (with default parameters)", { test_that("returns the correct linting (with 'allow_trailing' set)", { linter <- commas_linter(allow_trailing = TRUE) - msg_after <- rex::rex("Commas should always have a space after.") - msg_before <- rex::rex("Commas should never have a space before.") + msg_after <- rex::rex("Commas should always precede a space.") + msg_before <- rex::rex("Commas should never follow a space.") expect_lint("blah", NULL, linter) expect_lint("fun(1, 1)", NULL, linter) diff --git a/tests/testthat/test-commented_code_linter.R b/tests/testthat/test-commented_code_linter.R index 0d034e8195..d7ff746514 100644 --- a/tests/testthat/test-commented_code_linter.R +++ b/tests/testthat/test-commented_code_linter.R @@ -20,7 +20,7 @@ test_that("commented_code_linter skips allowed usages", { }) test_that("commented_code_linter blocks disallowed usages", { - lint_msg <- rex::rex("Commented code should be removed.") + lint_msg <- rex::rex("Remove commented code.") linter <- commented_code_linter() expect_lint("# blah <- 1", lint_msg, linter) @@ -80,7 +80,7 @@ test_that("commented_code_linter blocks disallowed usages", { test_that("commented_code_linter can detect operators in comments and lint correctly", { linter <- commented_code_linter() - lint_msg <- rex::rex("Commented code should be removed.") + lint_msg <- rex::rex("Remove commented code.") test_ops <- c( "+", "=", "==", "!=", "<=", ">=", "<-", "<<-", "<", ">", "->", @@ -100,7 +100,7 @@ test_that("commented_code_linter can detect operators in comments and lint corre expect_lint( "# 1:3 |> sum()", - rex::rex("Commented code should be removed."), + rex::rex("Remove commented code."), commented_code_linter() ) }) diff --git a/tests/testthat/test-condition_call_linter.R b/tests/testthat/test-condition_call_linter.R index 6ef37aa0cf..f0a595149b 100644 --- a/tests/testthat/test-condition_call_linter.R +++ b/tests/testthat/test-condition_call_linter.R @@ -20,13 +20,13 @@ patrick::with_parameters_test_that( "condition_call_linter blocks disallowed usages", { linter <- condition_call_linter() - lint_message <- rex::rex(call_name, anything, "to not display call") + lint_message <- rex::rex(call_name, anything, "not to display the call") expect_lint(paste0(call_name, "('test')"), lint_message, linter) expect_lint(paste0(call_name, "('test', call. = TRUE)"), lint_message, linter) linter <- condition_call_linter(display_call = TRUE) - lint_message <- rex::rex(call_name, anything, "to display call") + lint_message <- rex::rex(call_name, anything, "to display the call") expect_lint(paste0(call_name, "('test', call. = FALSE)"), lint_message, linter) diff --git a/tests/testthat/test-conjunct_test_linter.R b/tests/testthat/test-conjunct_test_linter.R index 918d8685ea..7a8a28986f 100644 --- a/tests/testthat/test-conjunct_test_linter.R +++ b/tests/testthat/test-conjunct_test_linter.R @@ -19,22 +19,16 @@ test_that("conjunct_test_linter skips allowed usages of expect_true", { }) test_that("conjunct_test_linter blocks && conditions with expect_true()", { - expect_lint( - "expect_true(x && y)", - rex::rex("Instead of expect_true(A && B), write multiple expectations"), - conjunct_test_linter() - ) + linter <- conjunct_test_linter() + lint_msg <- rex::rex("Write multiple expectations like expect_true(A) and expect_true(B) instead of expect_true(A && B)") - expect_lint( - "expect_true(x && y && z)", - rex::rex("Instead of expect_true(A && B), write multiple expectations"), - conjunct_test_linter() - ) + expect_lint("expect_true(x && y)", lint_msg, linter) + expect_lint("expect_true(x && y && z)", lint_msg, linter) }) test_that("conjunct_test_linter blocks || conditions with expect_false()", { linter <- conjunct_test_linter() - lint_msg <- rex::rex("Instead of expect_false(A || B), write multiple expectations") + lint_msg <- rex::rex("Write multiple expectations like expect_false(A) and expect_false(B) instead of expect_false(A || B)") expect_lint("expect_false(x || y)", lint_msg, linter) expect_lint("expect_false(x || y || z)", lint_msg, linter) @@ -59,7 +53,7 @@ test_that("conjunct_test_linter skips allowed stopifnot() and assert_that() usag test_that("conjunct_test_linter blocks simple disallowed usages of stopifnot() and assert_that()", { linter <- conjunct_test_linter() - lint_msg <- function(fun) rex::rex("Instead of ", fun, "(A && B), write multiple conditions") + lint_msg <- function(fun) rex::rex("Write multiple conditions like ", fun, "(A, B) instead of ", fun, "(A && B)") expect_lint("stopifnot(x && y)", lint_msg("stopifnot"), linter) expect_lint("stopifnot(x && y && z)", lint_msg("stopifnot"), linter) @@ -78,7 +72,7 @@ test_that("conjunct_test_linter's allow_named_stopifnot argument works", { ) expect_lint( "stopifnot('x is a logical scalar' = length(x) == 1 && is.logical(x) && !is.na(x))", - rex::rex("Instead of stopifnot(A && B), write multiple conditions"), + rex::rex("Write multiple conditions like stopifnot(A, B)"), conjunct_test_linter(allow_named_stopifnot = FALSE) ) }) diff --git a/tests/testthat/test-cyclocomp_linter.R b/tests/testthat/test-cyclocomp_linter.R index 9162c75c7c..3c298c659c 100644 --- a/tests/testthat/test-cyclocomp_linter.R +++ b/tests/testthat/test-cyclocomp_linter.R @@ -1,7 +1,7 @@ test_that("returns the correct linting", { cc_linter_1 <- cyclocomp_linter(1L) cc_linter_2 <- cyclocomp_linter(2L) - lint_msg <- rex::rex("Functions should have cyclomatic complexity") + lint_msg <- rex::rex("Reduce the cyclomatic complexity of this function") expect_lint("if (TRUE) 1 else 2", NULL, cc_linter_2) expect_lint("if (TRUE) 1 else 2", lint_msg, cc_linter_1) @@ -40,7 +40,7 @@ test_that("returns the correct linting", { expect_lint(complex_lines, lint_msg, cc_linter_2) expect_lint( complex_lines, - "should have cyclomatic complexity of less than 2, this has 10", + rex::rex("Reduce the cyclomatic complexity of this function from 10 to at most 2."), cc_linter_2 ) expect_lint(complex_lines, NULL, cyclocomp_linter(10L)) diff --git a/tests/testthat/test-duplicate_argument_linter.R b/tests/testthat/test-duplicate_argument_linter.R index 4c6bba76de..2ddd6da922 100644 --- a/tests/testthat/test-duplicate_argument_linter.R +++ b/tests/testthat/test-duplicate_argument_linter.R @@ -11,7 +11,7 @@ test_that("duplicate_argument_linter doesn't block allowed usages", { test_that("duplicate_argument_linter blocks disallowed usages", { linter <- duplicate_argument_linter() - lint_msg <- rex::rex("Duplicate arguments in function call.") + lint_msg <- rex::rex("Remove duplicate arguments in function call.") expect_lint("fun(arg = 1, arg = 2)", lint_msg, linter) expect_lint("fun(arg = 1, 'arg' = 2)", lint_msg, linter) @@ -51,13 +51,13 @@ test_that("duplicate_argument_linter respects except argument", { "fun(` ` = 1, ` ` = 2)", - list(message = rex::rex("Duplicate arguments in function call.")), + rex::rex("Remove duplicate arguments in function call."), duplicate_argument_linter(except = character()) ) expect_lint( "function(arg = 1, arg = 1) {}", - list(message = rex::rex("Repeated formal argument 'arg'.")), + rex::rex("Repeated formal argument 'arg'."), duplicate_argument_linter(except = character()) ) }) diff --git a/tests/testthat/test-fixed_regex_linter.R b/tests/testthat/test-fixed_regex_linter.R index 0fb6a8fb23..6e60bf2d27 100644 --- a/tests/testthat/test-fixed_regex_linter.R +++ b/tests/testthat/test-fixed_regex_linter.R @@ -245,8 +245,8 @@ test_that("fixed replacements vectorize and recognize str_detect", { ) "), list( - rex::rex('Here, you can use "abcdefg" with fixed = TRUE'), - rex::rex('Here, you can use "a..b\\n" with fixed = TRUE') + rex::rex('Use "abcdefg" with fixed = TRUE'), + rex::rex('Use "a..b\\n" with fixed = TRUE') ), linter ) @@ -254,7 +254,7 @@ test_that("fixed replacements vectorize and recognize str_detect", { # stringr hint works expect_lint( "str_detect(x, 'abc')", - rex::rex('Here, you can use stringr::fixed("abc") as the pattern'), + rex::rex('Use stringr::fixed("abc") as the pattern'), linter ) }) @@ -267,7 +267,7 @@ test_that("fixed replacement is correct with UTF-8", { expect_lint( "grepl('[\\U{1D4D7}]', x)", - rex::rex('Here, you can use "\U1D4D7" with fixed = TRUE'), + rex::rex('Use "\U1D4D7" with fixed = TRUE'), fixed_regex_linter() ) }) @@ -311,7 +311,7 @@ patrick::with_parameters_test_that("fixed replacements are correct", { ) expect_lint( sprintf("grepl('%s', x)", regex_expr), - rex::rex(sprintf('Here, you can use "%s" with fixed = TRUE', fixed_expr)), + rex::rex(sprintf('Use "%s" with fixed = TRUE', fixed_expr)), fixed_regex_linter() ) }, .cases = tibble::tribble( @@ -354,7 +354,7 @@ test_that("'unescaped' regex can optionally be skipped", { expect_lint("grepl('a', x)", NULL, linter) expect_lint("str_detect(x, 'a')", NULL, linter) - expect_lint("grepl('[$]', x)", rex::rex('Here, you can use "$"'), linter) + expect_lint("grepl('[$]', x)", rex::rex('Use "$" with fixed = TRUE'), linter) }) local({ diff --git a/tests/testthat/test-if_not_else_linter.R b/tests/testthat/test-if_not_else_linter.R index 27f0b1df2a..35d8aac181 100644 --- a/tests/testthat/test-if_not_else_linter.R +++ b/tests/testthat/test-if_not_else_linter.R @@ -20,7 +20,7 @@ test_that("if_not_else_linter skips allowed usages", { test_that("if_not_else_linter blocks simple disallowed usages", { linter <- if_not_else_linter() - lint_msg <- rex::rex("In a simple if/else statement, prefer `if (A) x else y`") + lint_msg <- rex::rex("Prefer `if (A) x else y`") expect_lint("if (!A) x else y", lint_msg, linter) expect_lint("if (!A) x else if (!B) y else z", lint_msg, linter) @@ -65,7 +65,7 @@ test_that("multiple lints are generated correctly", { if_else(!A, x, y) }"), list( - "In a simple if/else statement", + rex::rex("Prefer `if (A) x else y`"), "Prefer `ifelse", "Prefer `fifelse", "Prefer `if_else" @@ -77,7 +77,7 @@ test_that("multiple lints are generated correctly", { test_that("exceptions= argument works", { expect_lint( "if (!is.null(x)) x else y", - "In a simple if/else statement", + rex::rex("Prefer `if (A) x else y`"), if_not_else_linter(exceptions = character()) ) diff --git a/tests/testthat/test-implicit_integer_linter.R b/tests/testthat/test-implicit_integer_linter.R index cd0bebec07..6449a54ce0 100644 --- a/tests/testthat/test-implicit_integer_linter.R +++ b/tests/testthat/test-implicit_integer_linter.R @@ -48,15 +48,15 @@ local({ linter <- implicit_integer_linter() patrick::with_parameters_test_that( "single numerical constants are properly identified ", - expect_lint(num_value_str, if (should_lint) "Integers should not be implicit", linter), + expect_lint(num_value_str, if (should_lint) "Avoid implicit integers", linter), .cases = cases ) }) # styler: on test_that("linter returns the correct linting", { - lint_msg <- "Integers should not be implicit. Use the form 1L for integers or 1.0 for doubles." linter <- implicit_integer_linter() + lint_msg <- rex::rex("Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.") expect_lint("x <<- 1L", NULL, linter) expect_lint("1.0/-Inf -> y", NULL, linter) @@ -90,7 +90,7 @@ patrick::with_parameters_test_that( "numbers in a:b input are optionally not linted", expect_lint( paste0(left, ":", right), - if (n_lints > 0L) rep(list("Integers should not be implicit"), n_lints), + if (n_lints > 0L) rep(list("Avoid implicit integers"), n_lints), implicit_integer_linter(allow_colon = allow_colon) ), .cases = tibble::tribble( diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index 95b2d69574..c3278a07f1 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -1,22 +1,26 @@ test_that("is_numeric_linter skips allowed usages involving ||", { - expect_lint("is.numeric(x) || is.integer(y)", NULL, is_numeric_linter()) + linter <- is_numeric_linter() + + expect_lint("is.numeric(x) || is.integer(y)", NULL, linter) # x is used, but not identically - expect_lint("is.numeric(x) || is.integer(foo(x))", NULL, is_numeric_linter()) + expect_lint("is.numeric(x) || is.integer(foo(x))", NULL, linter) # not totally crazy, e.g. if input accepts a vector or a list - expect_lint("is.numeric(x) || is.integer(x[[1]])", NULL, is_numeric_linter()) + expect_lint("is.numeric(x) || is.integer(x[[1]])", NULL, linter) }) test_that("is_numeric_linter skips allowed usages involving %in%", { + linter <- is_numeric_linter() + # false positives for class(x) %in% c('integer', 'numeric') style - expect_lint("class(x) %in% 1:10", NULL, is_numeric_linter()) - expect_lint("class(x) %in% 'numeric'", NULL, is_numeric_linter()) - expect_lint("class(x) %in% c('numeric', 'integer', 'factor')", NULL, is_numeric_linter()) - expect_lint("class(x) %in% c('numeric', 'integer', y)", NULL, is_numeric_linter()) + expect_lint("class(x) %in% 1:10", NULL, linter) + expect_lint("class(x) %in% 'numeric'", NULL, linter) + expect_lint("class(x) %in% c('numeric', 'integer', 'factor')", NULL, linter) + expect_lint("class(x) %in% c('numeric', 'integer', y)", NULL, linter) }) test_that("is_numeric_linter blocks disallowed usages involving ||", { linter <- is_numeric_linter() - lint_msg <- rex::rex("same as is.numeric(x) || is.integer(x)") + lint_msg <- rex::rex("Use `is.numeric(x)` instead of equivalent `is.numeric(x) || is.integer(x)`.") expect_lint("is.numeric(x) || is.integer(x)", lint_msg, linter) @@ -44,7 +48,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { test_that("is_numeric_linter blocks disallowed usages involving %in%", { linter <- is_numeric_linter() - lint_msg <- rex::rex('same as class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c('integer', 'numeric')", lint_msg, linter) expect_lint('class(x) %in% c("numeric", "integer")', lint_msg, linter) @@ -54,7 +58,7 @@ test_that("raw strings are handled properly when testing in class", { skip_if_not_r_version("4.0.0") linter <- is_numeric_linter() - lint_msg <- rex::rex('same as class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c(R'(numeric)', 'integer', 'factor')", NULL, linter) expect_lint("class(x) %in% c('numeric', R'--(integer)--', y)", NULL, linter) diff --git a/tests/testthat/test-knitr_formats.R b/tests/testthat/test-knitr_formats.R index 7ae3f63f7a..1634199bde 100644 --- a/tests/testthat/test-knitr_formats.R +++ b/tests/testthat/test-knitr_formats.R @@ -2,8 +2,8 @@ regexes <- list( assign = rex::rex("Use <-, not =, for assignment."), local_var = rex::rex("local variable"), quotes = rex::rex("Only use double-quotes."), - trailing = rex::rex("Trailing blank lines are superfluous."), - trailws = rex::rex("Trailing whitespace is superfluous."), + trailing = rex::rex("Remove trailing blank lines."), + trailws = rex::rex("Remove trailing whitespace."), indent = rex::rex("Indentation should be") ) diff --git a/tests/testthat/test-nested_ifelse_linter.R b/tests/testthat/test-nested_ifelse_linter.R index 14bd3302f5..a7bd0df687 100644 --- a/tests/testthat/test-nested_ifelse_linter.R +++ b/tests/testthat/test-nested_ifelse_linter.R @@ -14,13 +14,13 @@ test_that("nested_ifelse_linter skips allowed usages", { test_that("nested_ifelse_linter blocks simple disallowed usages", { expect_lint( "ifelse(l1, v1, ifelse(l2, v2, v3))", - rex::rex("Don't use nested ifelse() calls"), + rex::rex("Avoid nested ifelse() calls"), nested_ifelse_linter() ) expect_lint( "ifelse(l1, ifelse(l2, v1, v2), v3)", - rex::rex("Don't use nested ifelse() calls"), + rex::rex("Avoid nested ifelse() calls"), nested_ifelse_linter() ) }) @@ -28,13 +28,13 @@ test_that("nested_ifelse_linter blocks simple disallowed usages", { test_that("nested_ifelse_linter also catches dplyr::if_else", { expect_lint( "if_else(l1, v1, if_else(l2, v2, v3))", - rex::rex("Don't use nested if_else() calls"), + rex::rex("Avoid nested if_else() calls"), nested_ifelse_linter() ) expect_lint( "dplyr::if_else(l1, dplyr::if_else(l2, v1, v2), v3)", - rex::rex("Don't use nested if_else() calls"), + rex::rex("Avoid nested if_else() calls"), nested_ifelse_linter() ) }) @@ -42,20 +42,20 @@ test_that("nested_ifelse_linter also catches dplyr::if_else", { test_that("nested_ifelse_linter also catches data.table::fifelse", { expect_lint( "fifelse(l1, v1, fifelse(l2, v2, v3))", - rex::rex("Don't use nested fifelse() calls"), + rex::rex("Avoid nested fifelse() calls"), nested_ifelse_linter() ) expect_lint( "data.table::fifelse(l1, v1, data.table::fifelse(l2, v2, v3))", - rex::rex("Don't use nested fifelse() calls"), + rex::rex("Avoid nested fifelse() calls"), nested_ifelse_linter() ) # not sure why anyone would do this, but the readability still argument holds expect_lint( "data.table::fifelse(l1, dplyr::if_else(l2, v1, v2), v3)", - rex::rex("Don't use nested if_else() calls"), + rex::rex("Avoid nested if_else() calls"), nested_ifelse_linter() ) }) diff --git a/tests/testthat/test-nested_pipe_linter.R b/tests/testthat/test-nested_pipe_linter.R index 1e16792385..3072d3ced3 100644 --- a/tests/testthat/test-nested_pipe_linter.R +++ b/tests/testthat/test-nested_pipe_linter.R @@ -58,7 +58,7 @@ patrick::with_parameters_test_that( test_that("nested_pipe_linter blocks simple disallowed usages", { linter <- nested_pipe_linter() linter_inline <- nested_pipe_linter(allow_inline = FALSE) - lint_msg <- rex::rex("Don't nest pipes inside other calls.") + lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") expect_lint( "bind_rows(a %>% select(b), c %>% select(b))", @@ -110,7 +110,7 @@ test_that("allow_outer_calls= argument works", { foo() ) "), - rex::rex("Don't nest pipes inside other calls."), + rex::rex("Avoid nesting pipes inside other calls."), nested_pipe_linter(allow_outer_calls = character()) ) @@ -131,7 +131,7 @@ test_that("Native pipes are handled as well", { linter <- nested_pipe_linter() linter_inline <- nested_pipe_linter(allow_inline = FALSE) - lint_msg <- rex::rex("Don't nest pipes inside other calls.") + lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") expect_lint( "bind_rows(a |> select(b), c |> select(b))", @@ -157,7 +157,7 @@ test_that("Native pipes are handled as well", { }) test_that("lints vectorize", { - lint_msg <- rex::rex("Don't nest pipes inside other calls.") + lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") lines <- trim_some("{ bind_rows( diff --git a/tests/testthat/test-nzchar_linter.R b/tests/testthat/test-nzchar_linter.R index 4a3c65dca6..1e6eb9260f 100644 --- a/tests/testthat/test-nzchar_linter.R +++ b/tests/testthat/test-nzchar_linter.R @@ -25,15 +25,15 @@ test_that("nzchar_linter skips as appropriate for other nchar args", { # nzchar also has keepNA argument so a drop-in switch is easy expect_lint( "nchar(x, keepNA=TRUE) == 0", - rex::rex("Instead of comparing nchar(x) to 0"), + rex::rex("Use nzchar() instead of comparing nchar(x) to 0"), linter ) }) test_that("nzchar_linter blocks simple disallowed usages", { linter <- nzchar_linter() - lint_msg_quote <- rex::rex('Instead of comparing strings to "", use nzchar()') - lint_msg_nchar <- rex::rex("Instead of comparing nchar(x) to 0") + lint_msg_quote <- rex::rex('Use nzchar() instead of comparing strings to ""') + lint_msg_nchar <- rex::rex("Use nzchar() instead of comparing nchar(x) to 0") expect_lint("which(x == '')", lint_msg_quote, linter) expect_lint("any(nchar(x) >= 0)", lint_msg_nchar, linter) @@ -43,8 +43,8 @@ test_that("nzchar_linter blocks simple disallowed usages", { test_that("nzchar_linter skips comparison to '' in if/while statements", { linter <- nzchar_linter() - lint_msg_quote <- rex::rex('Instead of comparing strings to "", use nzchar()') - lint_msg_nchar <- rex::rex("Instead of comparing nchar(x) to 0") + lint_msg_quote <- rex::rex('Use nzchar() instead of comparing strings to ""') + lint_msg_nchar <- rex::rex("Use nzchar() instead of comparing nchar(x) to 0") # still lint nchar() comparisons expect_lint("if (nchar(x) > 0) TRUE", lint_msg_nchar, linter) @@ -66,8 +66,8 @@ test_that("multiple lints are generated correctly", { nchar(b) != 0 }"), list( - list(rex::rex('Instead of comparing strings to ""'), line_number = 2L), - list(rex::rex("Instead of comparing nchar(x) to 0"), line_number = 3L) + list(rex::rex('Use nzchar() instead of comparing strings to ""'), line_number = 2L), + list(rex::rex("Use nzchar() instead of comparing nchar(x) to 0."), line_number = 3L) ), nzchar_linter() ) diff --git a/tests/testthat/test-one_call_pipe_linter.R b/tests/testthat/test-one_call_pipe_linter.R index a24f3380ba..5cd772c912 100644 --- a/tests/testthat/test-one_call_pipe_linter.R +++ b/tests/testthat/test-one_call_pipe_linter.R @@ -14,7 +14,7 @@ test_that("one_call_pipe_linter skips allowed usages", { test_that("one_call_pipe_linter blocks simple disallowed usages", { linter <- one_call_pipe_linter() - lint_msg <- rex::rex("Expressions with only a single call shouldn't use pipe %>%.") + lint_msg <- rex::rex("Avoid pipe %>% for expressions with only a single call.") expect_lint("x %>% foo()", lint_msg, linter) @@ -29,7 +29,7 @@ test_that("one_call_pipe_linter blocks simple disallowed usages", { test_that("one_call_pipe_linter skips data.table chains", { linter <- one_call_pipe_linter() - lint_msg <- rex::rex("Expressions with only a single call shouldn't use pipe %>%.") + lint_msg <- rex::rex("Avoid pipe %>% for expressions with only a single call.") expect_lint("DT[x > 5, sum(y), by = keys] %>% .[, .SD[1], by = key1]", NULL, linter) @@ -44,11 +44,11 @@ test_that("one_call_pipe_linter skips data.table chains", { test_that("one_call_pipe_linter treats all pipes equally", { linter <- one_call_pipe_linter() - lint_msg_part <- "Expressions with only a single call shouldn't use pipe " + lint_msg_part <- " for expressions with only a single call." expect_lint("foo %>% bar() %$% col", NULL, linter) - expect_lint("x %T>% foo()", rex::rex(lint_msg_part, "%T>%."), linter) - expect_lint("x %$%\n foo", rex::rex(lint_msg_part, "%$%."), linter) + expect_lint("x %T>% foo()", rex::rex("%T>%", lint_msg_part), linter) + expect_lint("x %$%\n foo", rex::rex("%$%", lint_msg_part), linter) expect_lint( 'data %>% filter(type == "console") %$% obscured_id %>% unique()', NULL, @@ -80,7 +80,7 @@ test_that("Native pipes are handled as well", { expect_lint( "x |> foo()", - rex::rex("Expressions with only a single call shouldn't use pipe |>."), + rex::rex("Avoid pipe |> for expressions with only a single call."), linter ) @@ -105,7 +105,7 @@ test_that("one_call_pipe_linter skips data.table chains with native pipe", { skip_if_not_r_version("4.3.0") linter <- one_call_pipe_linter() - lint_msg <- rex::rex("Expressions with only a single call shouldn't use pipe |>.") + lint_msg <- rex::rex("Avoid pipe |> for expressions with only a single call.") expect_lint("DT[x > 5, sum(y), by = keys] |> _[, .SD[1], by = key1]", NULL, linter) diff --git a/tests/testthat/test-paren_body_linter.R b/tests/testthat/test-paren_body_linter.R index 64e2522b3e..dac02cae47 100644 --- a/tests/testthat/test-paren_body_linter.R +++ b/tests/testthat/test-paren_body_linter.R @@ -1,6 +1,6 @@ testthat::test_that("paren_body_linter returns correct lints", { linter <- paren_body_linter() - lint_msg <- "There should be a space between a right parenthesis and a body expression." + lint_msg <- rex::rex("Put a space between a right parenthesis and a body expression.") # No space after the closing parenthesis prompts a lint expect_lint("function()test", lint_msg, linter) @@ -49,29 +49,32 @@ testthat::test_that("paren_body_linter returns correct lints", { }) test_that("multi-line versions are caught", { + linter <- paren_body_linter() + lint_msg <- rex::rex("Put a space between a right parenthesis and a body expression.") + expect_lint( trim_some(" function(var )x "), - rex::rex("There should be a space between a right parenthesis and a body expression."), - paren_body_linter() + lint_msg, + linter ) expect_lint( trim_some(" if (cond )x "), - rex::rex("There should be a space between a right parenthesis and a body expression."), - paren_body_linter() + lint_msg, + linter ) expect_lint( trim_some(" while (cond )x "), - rex::rex("There should be a space between a right parenthesis and a body expression."), - paren_body_linter() + lint_msg, + linter ) skip_if_not_r_version("4.1.0") @@ -80,15 +83,15 @@ test_that("multi-line versions are caught", { \\(var )x "), - rex::rex("There should be a space between a right parenthesis and a body expression."), - paren_body_linter() + lint_msg, + linter ) }) test_that("function shorthand is handled", { skip_if_not_r_version("4.1.0") linter <- paren_body_linter() - lint_msg <- rex::rex("There should be a space between a right parenthesis and a body expression.") + lint_msg <- rex::rex("Put a space between a right parenthesis and a body expression.") expect_lint("\\()test", lint_msg, linter) }) diff --git a/tests/testthat/test-pipe-consistency-linter.R b/tests/testthat/test-pipe-consistency-linter.R index 2406e98318..4a236b156d 100644 --- a/tests/testthat/test-pipe-consistency-linter.R +++ b/tests/testthat/test-pipe-consistency-linter.R @@ -21,7 +21,7 @@ test_that("pipe_consistency skips allowed usage", { test_that("pipe_consistency lints inconsistent usage", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter() - expected_msg <- rex("Found 1 instances of %>% and 1 instances of |>. Stick to one pipe operator.") + expected_msg <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 1 instances of |>.") expect_lint( "1:3 |> mean() %>% as.character()", @@ -54,7 +54,7 @@ test_that("pipe_consistency lints inconsistent usage", { linter ) - expected_msg_multi <- rex("Found 1 instances of %>% and 2 instances of |>. Stick to one pipe operator.") + expected_msg_multi <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 2 instances of |>.") expect_lint( "1:3 |> sort() |> mean() %>% as.character()", list( @@ -71,7 +71,7 @@ test_that("pipe_consistency_linter works with |> argument", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter(pipe = "|>") - expected_message <- rex("Use the |> pipe operator instead of the %>% pipe operator.") + expected_message <- rex::rex("Use the |> pipe operator instead of the %>% pipe operator.") expect_lint( trim_some(" @@ -117,7 +117,7 @@ test_that("pipe_consistency_linter works with %>% argument", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter(pipe = "%>%") - expected_message <- rex("Use the %>% pipe operator instead of the |> pipe operator.") + expected_message <- rex::rex("Use the %>% pipe operator instead of the |> pipe operator.") expect_lint( "1:3 |> mean() |> as.character()", @@ -154,7 +154,7 @@ test_that("pipe_consistency_linter works with %>% argument", { test_that("pipe_consistency_linter works with other magrittr pipes", { skip_if_not_r_version("4.1.0") linter <- pipe_consistency_linter() - expected_message <- rex("Found 1 instances of %>% and 1 instances of |>. Stick to one pipe operator.") + expected_message <- rex::rex("Stick to one pipe operator; found 1 instances of %>% and 1 instances of |>.") expect_lint("1:3 %>% mean() %T% print()", NULL, linter) expect_lint( diff --git a/tests/testthat/test-pipe_continuation_linter.R b/tests/testthat/test-pipe_continuation_linter.R index 5f8fd48691..528f0c0118 100644 --- a/tests/testthat/test-pipe_continuation_linter.R +++ b/tests/testthat/test-pipe_continuation_linter.R @@ -1,6 +1,6 @@ test_that("pipe-continuation correctly handles stand-alone expressions", { linter <- pipe_continuation_linter() - lint_msg <- rex::rex("`%>%` should always have a space before it and a new line after it,") + lint_msg <- rex::rex("Put a space before `%>%` and a new line after it,") # Expressions without pipes are ignored expect_lint("blah", NULL, linter) @@ -41,7 +41,7 @@ test_that("pipe-continuation correctly handles stand-alone expressions", { test_that("pipe-continuation linter correctly handles nesting", { linter <- pipe_continuation_linter() - lint_msg <- rex::rex("`%>%` should always have a space before it and a new line after it,") + lint_msg <- rex::rex("Put a space before `%>%` and a new line after it,") expect_lint( trim_some(" @@ -81,8 +81,8 @@ test_that("pipe-continuation linter handles native pipe", { skip_if_not_r_version("4.1.0") linter <- pipe_continuation_linter() - lint_msg_native <- rex::rex("`|>` should always have a space before it and a new line after it,") - lint_msg_magrittr <- rex::rex("`%>%` should always have a space before it and a new line after it,") + lint_msg_native <- rex::rex("Put a space before `|>` and a new line after it,") + lint_msg_magrittr <- rex::rex("Put a space before `%>%` and a new line after it,") expect_lint("foo |> bar() |> baz()", NULL, linter) expect_lint( @@ -201,7 +201,7 @@ local({ "Various pipes are linted correctly", expect_lint( sprintf("a %s b() %s\n c()", pipe1, pipe2), - rex::rex(sprintf("`%s` should always have a space before it", pipe2)), + rex::rex(sprintf("Put a space before `%s` and a new line after it", pipe2)), linter ), .cases = cases diff --git a/tests/testthat/test-semicolon_linter.R b/tests/testthat/test-semicolon_linter.R index ea809df377..6cb5dd5381 100644 --- a/tests/testthat/test-semicolon_linter.R +++ b/tests/testthat/test-semicolon_linter.R @@ -1,7 +1,7 @@ test_that("Lint all semicolons", { linter <- semicolon_linter() - trail_msg <- "Trailing semicolons are not needed." - comp_msg <- "Compound semicolons are discouraged. Replace them by a newline." + trail_msg <- rex::rex("Remove trailing semicolons.") + comp_msg <- rex::rex("Replace compound semicolons by a newline.") # No semicolon expect_lint("", NULL, linter) diff --git a/tests/testthat/test-seq_linter.R b/tests/testthat/test-seq_linter.R index d04e40e158..7edaee13e8 100644 --- a/tests/testthat/test-seq_linter.R +++ b/tests/testthat/test-seq_linter.R @@ -20,25 +20,25 @@ test_that("finds seq(...) expressions", { expect_lint( "function(x) { seq(length(x)) }", - rex::rex("seq(length(...))", anything, "Use seq_along(...)"), + rex::rex("Use seq_along(...) instead of seq(length(...))"), linter ) expect_lint( "function(x) { seq(nrow(x)) }", - rex::rex("seq(nrow(...))", anything, "Use seq_len(nrow(...))"), + rex::rex("Use seq_len(nrow(...)) instead of seq(nrow(...))"), linter ) expect_lint( "function(x) { rev(seq(length(x))) }", - rex::rex("seq(length(...))", anything, "Use seq_along(...)"), + rex::rex("Use seq_along(...) instead of seq(length(...))"), linter ) expect_lint( "function(x) { rev(seq(nrow(x))) }", - rex::rex("seq(nrow(...))", anything, "Use seq_len(nrow(...))"), + rex::rex("Use seq_len(nrow(...)) instead of seq(nrow(...))"), linter ) }) @@ -48,19 +48,19 @@ test_that("finds 1:length(...) expressions", { expect_lint( "function(x) { 1:length(x) }", - rex::rex("length(...)", anything, "Use seq_along"), + rex::rex("Use seq_along(...) instead of 1:length(...)"), linter ) expect_lint( "function(x) { 1:nrow(x) }", - rex::rex("nrow(...)", anything, "Use seq_len"), + rex::rex("Use seq_len(nrow(...)) instead of 1:nrow(...)"), linter ) expect_lint( "function(x) { 1:ncol(x) }", - rex::rex("ncol(...)", anything, "Use seq_len"), + rex::rex("Use seq_len(ncol(...)) instead of 1:ncol(...)"), linter ) From d45c3b6630b4b8faeecb7c7513fc5f81c194700a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 13:34:03 +0000 Subject: [PATCH 07/30] done tests? --- R/namespace_linter.R | 2 +- tests/testthat/test-assignment_linter.R | 19 +++++++++++-------- tests/testthat/test-brace_linter.R | 2 +- tests/testthat/test-class_equals_linter.R | 8 ++++---- tests/testthat/test-namespace_linter.R | 10 +++++----- tests/testthat/test-pipe_return_linter.R | 2 +- 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/R/namespace_linter.R b/R/namespace_linter.R index 7dd32ffffe..24fd465029 100644 --- a/R/namespace_linter.R +++ b/R/namespace_linter.R @@ -157,7 +157,7 @@ build_ns_get_int_lints <- function(packages, symbols, symbol_nodes, namespaces, symbol_nodes[exported], source_expression = source_expression, lint_message = - sprintf("Use %2$s::%1$s, not %2$s::%1$s.", symbols[exported], packages[exported]), + sprintf("Use %2$s::%1$s, not %2$s:::%1$s.", symbols[exported], packages[exported]), type = "warning" ) diff --git a/tests/testthat/test-assignment_linter.R b/tests/testthat/test-assignment_linter.R index f46067348a..6bf192b0ce 100644 --- a/tests/testthat/test-assignment_linter.R +++ b/tests/testthat/test-assignment_linter.R @@ -27,14 +27,17 @@ test_that("assignment_linter blocks disallowed usages", { }) test_that("arguments handle <<- and ->/->> correctly", { - expect_lint("1 -> blah", rex::rex("Use <-, not ->, for assignment."), assignment_linter()) - expect_lint("1 ->> blah", rex::rex("->> can have hard-to-predict behavior;"), assignment_linter()) + linter <- assignment_linter() + lint_msg_right <- rex::rex("Replace ->> by assigning to a specific environment") + + expect_lint("1 -> blah", rex::rex("Use <-, not ->, for assignment."), linter) + expect_lint("1 ->> blah", lint_msg_right, linter) # <<- is only blocked optionally - expect_lint("1 <<- blah", NULL, assignment_linter()) + expect_lint("1 <<- blah", NULL, linter) expect_lint( "1 <<- blah", - rex::rex("<<- can have hard-to-predict behavior;"), + rex::rex("Replace <<- by assigning to a specific environment"), assignment_linter(allow_cascading_assign = FALSE) ) @@ -44,7 +47,7 @@ test_that("arguments handle <<- and ->/->> correctly", { # blocked under cascading assign but not under right assign --> blocked expect_lint( "1 ->> blah", - rex::rex("->> can have hard-to-predict behavior;"), + lint_msg_right, assignment_linter(allow_cascading_assign = FALSE, allow_right_assign = TRUE) ) }) @@ -66,7 +69,7 @@ test_that("arguments handle trailing assignment operators correctly", { ) expect_lint( "x <<-\ny", - rex::rex("<<- can have hard-to-predict behavior"), + rex::rex("Replace <<- by assigning to a specific environment"), assignment_linter(allow_trailing = FALSE, allow_cascading_assign = FALSE) ) @@ -176,8 +179,8 @@ test_that("multiple lints throw correct messages", { expect_lint( "{ x <<- 1; y ->> 2; z -> 3; x %<>% as.character() }", list( - list(message = "<<- can have hard-to-predict behavior"), - list(message = "->> can have hard-to-predict behavior"), + list(message = "Replace <<- by assigning to a specific environment"), + list(message = "Replace ->> by assigning to a specific environment"), list(message = "Use <-, not ->"), list(message = "Avoid the assignment pipe %<>%") ), diff --git a/tests/testthat/test-brace_linter.R b/tests/testthat/test-brace_linter.R index e3adc1a599..3727e4c96d 100644 --- a/tests/testthat/test-brace_linter.R +++ b/tests/testthat/test-brace_linter.R @@ -315,7 +315,7 @@ test_that("brace_linter lints function expressions correctly", { ") expect_lint( lines, - rex::rex("Any function spanning multiple lines should use curly braces."), + rex::rex("Use curly braces for any function spanning multiple lines."), linter ) }) diff --git a/tests/testthat/test-class_equals_linter.R b/tests/testthat/test-class_equals_linter.R index d85c580081..d406e4a760 100644 --- a/tests/testthat/test-class_equals_linter.R +++ b/tests/testthat/test-class_equals_linter.R @@ -11,7 +11,7 @@ test_that("class_equals_linter skips allowed usages", { test_that("class_equals_linter blocks simple disallowed usages", { linter <- class_equals_linter() - lint_msg <- rex::rex("Instead of comparing class(x) with ==") + lint_msg <- rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')") expect_lint("if (class(x) == 'character') stop('no')", lint_msg, linter) expect_lint("is_regression <- class(x) == 'lm'", lint_msg, linter) @@ -20,7 +20,7 @@ test_that("class_equals_linter blocks simple disallowed usages", { test_that("class_equals_linter blocks usage of %in% for checking class", { linter <- class_equals_linter() - lint_msg <- rex::rex("Instead of comparing class(x) with %in%") + lint_msg <- rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')") expect_lint("if ('character' %in% class(x)) stop('no')", lint_msg, linter) expect_lint("if (class(x) %in% 'character') stop('no')", lint_msg, linter) @@ -29,7 +29,7 @@ test_that("class_equals_linter blocks usage of %in% for checking class", { test_that("class_equals_linter blocks class(x) != 'klass'", { expect_lint( "if (class(x) != 'character') TRUE", - rex::rex("Instead of comparing class(x) with !="), + rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')"), class_equals_linter() ) }) @@ -43,7 +43,7 @@ test_that("class_equals_linter skips usage for subsetting", { # but not further nesting expect_lint( "x[if (class(x) == 'foo') 1 else 2]", - rex::rex("Instead of comparing class(x) with =="), + rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')"), linter ) }) diff --git a/tests/testthat/test-namespace_linter.R b/tests/testthat/test-namespace_linter.R index fd3686fc82..d311cc74de 100644 --- a/tests/testthat/test-namespace_linter.R +++ b/tests/testthat/test-namespace_linter.R @@ -40,31 +40,31 @@ test_that("namespace_linter blocks disallowed usages", { expect_lint( "statts::sd(c(1,2,3))", - list(message = rex::rex("Package 'statts' is not installed.")), + rex::rex("Package 'statts' is not installed."), linter ) expect_lint( "stats::ssd(c(1,2,3))", - list(message = rex::rex("'ssd' is not exported from {stats}")), + rex::rex("'ssd' is not exported from {stats}"), linter ) expect_lint( "stats:::sd(c(1,2,3))", - list(message = rex::rex("'sd' is exported from {stats}. Use stats::sd instead.")), + rex::rex("Use stats::sd, not stats:::sd"), linter ) expect_lint( "statts:::sd(c(1,2,3))", - list(message = rex::rex("Package 'statts' is not installed.")), + rex::rex("Package 'statts' is not installed."), linter ) expect_lint( "stats:::sdd(c(1,2,3))", - list(message = rex::rex("'sdd' does not exist in {stats}")), + rex::rex("'sdd' does not exist in {stats}"), linter ) diff --git a/tests/testthat/test-pipe_return_linter.R b/tests/testthat/test-pipe_return_linter.R index 0c395d48b1..417ab4f984 100644 --- a/tests/testthat/test-pipe_return_linter.R +++ b/tests/testthat/test-pipe_return_linter.R @@ -41,7 +41,7 @@ test_that("pipe_return_linter blocks simple disallowed usages", { ") expect_lint( lines, - rex::rex("Using return() as the final step of a magrittr pipeline"), + rex::rex("Avoid return() as the final step of a magrittr pipeline"), pipe_return_linter() ) }) From 7d8f7229f825fd9e4079a3f7091ea938d5b4439a Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 13:48:48 +0000 Subject: [PATCH 08/30] fix expect_lint exxample --- R/expect_lint.R | 10 +++++----- man/expect_lint.Rd | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/expect_lint.R b/R/expect_lint.R index 5aabe7856b..2cc8e1f9e8 100644 --- a/R/expect_lint.R +++ b/R/expect_lint.R @@ -25,16 +25,16 @@ #' expect_lint("a", NULL, trailing_blank_lines_linter()) #' #' # one expected lint -#' expect_lint("a\n", "superfluous", trailing_blank_lines_linter()) -#' expect_lint("a\n", list(message = "superfluous", line_number = 2), trailing_blank_lines_linter()) +#' expect_lint("a\n", "trailing blank", trailing_blank_lines_linter()) +#' expect_lint("a\n", list(message = "trailing blank", line_number = 2), trailing_blank_lines_linter()) #' #' # several expected lints -#' expect_lint("a\n\n", list("superfluous", "superfluous"), trailing_blank_lines_linter()) +#' expect_lint("a\n\n", list("trailing blank", "trailing blank"), trailing_blank_lines_linter()) #' expect_lint( #' "a\n\n", #' list( -#' list(message = "superfluous", line_number = 2), -#' list(message = "superfluous", line_number = 3) +#' list(message = "trailing blank", line_number = 2), +#' list(message = "trailing blank", line_number = 3) #' ), #' trailing_blank_lines_linter() #' ) diff --git a/man/expect_lint.Rd b/man/expect_lint.Rd index 2832f9990e..8b7a22fc10 100644 --- a/man/expect_lint.Rd +++ b/man/expect_lint.Rd @@ -40,16 +40,16 @@ This is an expectation function to test that the lints produced by \code{lint} s expect_lint("a", NULL, trailing_blank_lines_linter()) # one expected lint -expect_lint("a\n", "superfluous", trailing_blank_lines_linter()) -expect_lint("a\n", list(message = "superfluous", line_number = 2), trailing_blank_lines_linter()) +expect_lint("a\n", "trailing blank", trailing_blank_lines_linter()) +expect_lint("a\n", list(message = "trailing blank", line_number = 2), trailing_blank_lines_linter()) # several expected lints -expect_lint("a\n\n", list("superfluous", "superfluous"), trailing_blank_lines_linter()) +expect_lint("a\n\n", list("trailing blank", "trailing blank"), trailing_blank_lines_linter()) expect_lint( "a\n\n", list( - list(message = "superfluous", line_number = 2), - list(message = "superfluous", line_number = 3) + list(message = "trailing blank", line_number = 2), + list(message = "trailing blank", line_number = 3) ), trailing_blank_lines_linter() ) From 74ae6623ec34c12c9dcba05c52e42e72cbff639e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 13:50:23 +0000 Subject: [PATCH 09/30] delint --- R/unnecessary_concatenation_linter.R | 2 +- tests/testthat/test-conjunct_test_linter.R | 6 ++++-- tests/testthat/test-implicit_integer_linter.R | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/R/unnecessary_concatenation_linter.R b/R/unnecessary_concatenation_linter.R index d33e28cecb..05a4f51aec 100644 --- a/R/unnecessary_concatenation_linter.R +++ b/R/unnecessary_concatenation_linter.R @@ -60,7 +60,7 @@ unnecessary_concatenation_linter <- function(allow_single_expression = TRUE) { # msg_empty <- "Replace unnecessary c() by NULL or, whenever possible, vector() seeded with the correct type and/or length." - msg_const <- 'Remove unnecessary c() of a constant.' + msg_const <- "Remove unnecessary c() of a constant." non_constant_cond <- "SYMBOL or (expr and not(OP-COLON and count(expr[SYMBOL or expr]) != 2))" diff --git a/tests/testthat/test-conjunct_test_linter.R b/tests/testthat/test-conjunct_test_linter.R index 7a8a28986f..355de5788e 100644 --- a/tests/testthat/test-conjunct_test_linter.R +++ b/tests/testthat/test-conjunct_test_linter.R @@ -20,7 +20,8 @@ test_that("conjunct_test_linter skips allowed usages of expect_true", { test_that("conjunct_test_linter blocks && conditions with expect_true()", { linter <- conjunct_test_linter() - lint_msg <- rex::rex("Write multiple expectations like expect_true(A) and expect_true(B) instead of expect_true(A && B)") + lint_msg <- + rex::rex("Write multiple expectations like expect_true(A) and expect_true(B) instead of expect_true(A && B)") expect_lint("expect_true(x && y)", lint_msg, linter) expect_lint("expect_true(x && y && z)", lint_msg, linter) @@ -28,7 +29,8 @@ test_that("conjunct_test_linter blocks && conditions with expect_true()", { test_that("conjunct_test_linter blocks || conditions with expect_false()", { linter <- conjunct_test_linter() - lint_msg <- rex::rex("Write multiple expectations like expect_false(A) and expect_false(B) instead of expect_false(A || B)") + lint_msg <- + rex::rex("Write multiple expectations like expect_false(A) and expect_false(B) instead of expect_false(A || B)") expect_lint("expect_false(x || y)", lint_msg, linter) expect_lint("expect_false(x || y || z)", lint_msg, linter) diff --git a/tests/testthat/test-implicit_integer_linter.R b/tests/testthat/test-implicit_integer_linter.R index 6449a54ce0..cddeff0447 100644 --- a/tests/testthat/test-implicit_integer_linter.R +++ b/tests/testthat/test-implicit_integer_linter.R @@ -56,7 +56,8 @@ local({ test_that("linter returns the correct linting", { linter <- implicit_integer_linter() - lint_msg <- rex::rex("Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.") + lint_msg <- + rex::rex("Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.") expect_lint("x <<- 1L", NULL, linter) expect_lint("1.0/-Inf -> y", NULL, linter) From fe5bd4c270283454c7e5fe7019b573bc58f9debc Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 13:53:26 +0000 Subject: [PATCH 10/30] NEWS --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 0fdee44e75..e499100b9d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,6 +11,7 @@ + Linters `closed_curly_linter()`, `open_curly_linter()`, `paren_brace_linter()`, and `semicolon_terminator_linter()`. + Helper `with_defaults()`. * `all_linters()` has signature `all_linters(..., packages)` rather than `all_linters(packages, ...)` (#2332, @MichaelChirico). This forces `packages=` to be supplied by name and will break users who rely on supplying `packages=` positionally, of which we found none searching GitHub. +* Adjusted various linters' messages for consistency in readability (#1330, @MichaelChirico). In general, we favor lint messages to be phrased like "Action, reason" to but the "what" piece of the message front-and-center. This may be a breaking change for code that tests the specific phrasing of lints. ## Bug fixes From 7eb4cf75c1404bd62944efdf93df1fab7bb9a212 Mon Sep 17 00:00:00 2001 From: Indrajeet Patil Date: Mon, 4 Dec 2023 18:36:28 +0100 Subject: [PATCH 11/30] correct spelling mistakes --- NEWS.md | 2 +- R/return_linter.R | 2 +- man/return_linter.Rd | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index e499100b9d..9ba894e624 100644 --- a/NEWS.md +++ b/NEWS.md @@ -16,7 +16,7 @@ ## Bug fixes * `object_name_linter()` no longer errors when user-supplied `regexes=` have capture groups (#2188, @MichaelChirico). -* `.lintr` config validation correctly accepts regular exressions which only compile under `perl = TRUE` (#2375, @MichaelChirico). These have always been valid (since `rex::re_matches()`, which powers the lint exclusion logic, also uses this setting), but the new up-front validation in v3.1.1 incorrectly used `perl = FALSE`. +* `.lintr` config validation correctly accepts regular expressions which only compile under `perl = TRUE` (#2375, @MichaelChirico). These have always been valid (since `rex::re_matches()`, which powers the lint exclusion logic, also uses this setting), but the new up-front validation in v3.1.1 incorrectly used `perl = FALSE`. ## Changes to default linters diff --git a/R/return_linter.R b/R/return_linter.R index 493d34d3d8..3127a2229a 100644 --- a/R/return_linter.R +++ b/R/return_linter.R @@ -3,7 +3,7 @@ #' This linter checks functions' [return()] expressions. #' #' @param return_style Character string naming the return style. `"implicit"`, -#' the default, enforeces the Tidyverse guide recommendation to leave terminal +#' the default, enforces the Tidyverse guide recommendation to leave terminal #' returns implicit. `"explicit"` style requires that `return()` always be #' explicitly supplied. #' @param return_functions Character vector of functions that are accepted as terminal calls diff --git a/man/return_linter.Rd b/man/return_linter.Rd index 4bcbd175e1..e4f5074ffc 100644 --- a/man/return_linter.Rd +++ b/man/return_linter.Rd @@ -12,7 +12,7 @@ return_linter( } \arguments{ \item{return_style}{Character string naming the return style. \code{"implicit"}, -the default, enforeces the Tidyverse guide recommendation to leave terminal +the default, enforces the Tidyverse guide recommendation to leave terminal returns implicit. \code{"explicit"} style requires that \code{return()} always be explicitly supplied.} From e5ad9dd1b0be0fe29513197cb9d9f4aa704ed0fb Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 17:11:58 -0800 Subject: [PATCH 12/30] Update R/class_equals_linter.R Co-authored-by: AshesITR --- R/class_equals_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/class_equals_linter.R b/R/class_equals_linter.R index 89c419522c..a210be31a2 100644 --- a/R/class_equals_linter.R +++ b/R/class_equals_linter.R @@ -52,7 +52,7 @@ class_equals_linter <- function() { operator <- xml_find_chr(bad_expr, "string(*[2])") lint_message <- sprintf( - "Use inherits(x, 'class-name') or is. or is(x, 'class') instead of comparing class(x) with %s.", + "Use inherits(x, 'class-name'), is. or is(x, 'class') instead of comparing class(x) with %s.", operator ) xml_nodes_to_lints( From 5d02db3af5618c4e3ebda18e242f2c397b512b56 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Mon, 4 Dec 2023 17:21:54 -0800 Subject: [PATCH 13/30] Update NEWS.md Co-authored-by: AshesITR --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 9ba894e624..81637440d5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -11,7 +11,7 @@ + Linters `closed_curly_linter()`, `open_curly_linter()`, `paren_brace_linter()`, and `semicolon_terminator_linter()`. + Helper `with_defaults()`. * `all_linters()` has signature `all_linters(..., packages)` rather than `all_linters(packages, ...)` (#2332, @MichaelChirico). This forces `packages=` to be supplied by name and will break users who rely on supplying `packages=` positionally, of which we found none searching GitHub. -* Adjusted various linters' messages for consistency in readability (#1330, @MichaelChirico). In general, we favor lint messages to be phrased like "Action, reason" to but the "what" piece of the message front-and-center. This may be a breaking change for code that tests the specific phrasing of lints. +* Adjusted various lint messages for consistency in readability (#1330, @MichaelChirico). In general, we favor lint messages to be phrased like "Action, reason" to but the "what" piece of the message front-and-center. This may be a breaking change for code that tests the specific phrasing of lints. ## Bug fixes From bf0bbb5249e4df91d23e3e1b9b273c1becb39200 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 01:30:22 +0000 Subject: [PATCH 14/30] simplify replacement/duplicate ifelse() --- R/fixed_regex_linter.R | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/R/fixed_regex_linter.R b/R/fixed_regex_linter.R index 47beb39a53..46f4a57b59 100644 --- a/R/fixed_regex_linter.R +++ b/R/fixed_regex_linter.R @@ -154,15 +154,14 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) { call_name <- xml_find_chr(patterns, "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)") is_stringr <- startsWith(call_name, "str_") - replacement <- ifelse( + replace_suggestion <- ifelse( is_stringr, - sprintf("stringr::fixed(%s)", fixed_equivalent), - fixed_equivalent + sprintf("stringr::fixed(%s) as the pattern", fixed_equivalent), + sprintf("%s with fixed = TRUE", fixed_equivalent) ) msg <- paste( - "Use", replacement, ifelse(is_stringr, "as the pattern", "with fixed = TRUE"), "here.", - "This regular expression is static, i.e., its matches can be expressed as a fixed substring expression, ", - "which is faster to compute." + "Use", replacement_suggestion, "here. This regular expression is static, i.e.,", + "its matches can be expressed as a fixed substring expression, which is faster to compute." ) xml_nodes_to_lints( From e714f73863d6b9dc6d198df54e77d6ac8c5b08a1 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 01:46:06 +0000 Subject: [PATCH 15/30] grammar --- R/is_numeric_linter.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R index 05ba934c80..2ca29871c8 100644 --- a/R/is_numeric_linter.R +++ b/R/is_numeric_linter.R @@ -78,7 +78,7 @@ is_numeric_linter <- function() { or_expr, source_expression = source_expression, lint_message = paste( - "Use `is.numeric(x)` instead of equivalent `is.numeric(x) || is.integer(x)`.", + "Use `is.numeric(x)` instead of the equivalent `is.numeric(x) || is.integer(x)`.", "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" @@ -97,7 +97,7 @@ is_numeric_linter <- function() { class_expr, source_expression = source_expression, lint_message = paste( - 'Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric").', + 'Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric").', "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" From 46d44f9c1fcb2f8d6106d9e6b3b6057e5e6ff808 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 01:46:45 +0000 Subject: [PATCH 16/30] calls with () --- R/nested_ifelse_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/nested_ifelse_linter.R b/R/nested_ifelse_linter.R index 0ce604e075..68d1d4905c 100644 --- a/R/nested_ifelse_linter.R +++ b/R/nested_ifelse_linter.R @@ -95,7 +95,7 @@ nested_ifelse_linter <- function() { matched_call <- xp_call_name(bad_expr) lint_message <- paste( sprintf("Avoid nested %s() calls.", matched_call), - "Instead, try (1) data.table::fcase; (2) dplyr::case_when; or (3) using a lookup table." + "Instead, try (1) data.table::fcase(); (2) dplyr::case_when(); or (3) using a lookup table." ) xml_nodes_to_lints(bad_expr, source_expression, lint_message, type = "warning") }) From c1c83ee8cb5ac79beb6fcbf0d433fab2f15341f0 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:02:16 +0000 Subject: [PATCH 17/30] condense --- .../test-trailing_blank_lines_linter.R | 36 +++++++------------ 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/tests/testthat/test-trailing_blank_lines_linter.R b/tests/testthat/test-trailing_blank_lines_linter.R index b5833e8a93..1775d78169 100644 --- a/tests/testthat/test-trailing_blank_lines_linter.R +++ b/tests/testthat/test-trailing_blank_lines_linter.R @@ -37,6 +37,7 @@ test_that("trailing_blank_lines_linter detects disallowed usages", { test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/qmd docs", { linter <- trailing_blank_lines_linter() + lint_msg <- rex::rex("Add a terminal newline") tmp3 <- withr::local_tempfile(fileext = ".Rmd") cat( @@ -56,12 +57,8 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ ) expect_lint( file = tmp3, - checks = list( - message = rex::rex("Add a terminal newline."), - line_number = 10L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + list(lint_msg, line_number = 10L, column_number = 1L), linters = linter ) @@ -79,12 +76,8 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ ) expect_lint( file = tmp4, - checks = list( - message = rex::rex("Add a terminal newline."), - line_number = 5L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + list(lint_msg, line_number = 5L, column_number = 1L), linters = linter ) @@ -107,18 +100,15 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ ) expect_lint( file = tmp5, - checks = list( - message = rex::rex("Add a terminal newline."), - line_number = 10L, - # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - column_number = 1L - ), + # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. + list(lint_msg, line_number = 10L, column_number = 1L), linters = linter ) }) test_that("blank lines in knitr chunks produce lints", { linter <- trailing_blank_lines_linter() + lint_msg <- rex::rex("Remove trailing blank lines.") tmp6 <- withr::local_tempfile( fileext = ".Rmd", @@ -137,7 +127,7 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp6, - checks = list(message = rex::rex("Remove trailing blank lines."), line_number = 7L, column_number = 1L), + list(lint_msg, line_number = 7L, column_number = 1L), linters = linter ) @@ -160,10 +150,10 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp7, - checks = list( - list(message = rex::rex("Remove trailing blank lines."), line_number = 7L, column_number = 1L), - list(message = rex::rex("Remove trailing blank lines."), line_number = 8L, column_number = 1L), - list(message = rex::rex("Remove trailing blank lines."), line_number = 9L, column_number = 1L) + list( + list(lint_msg, line_number = 7L, column_number = 1L), + list(lint_msg, line_number = 8L, column_number = 1L), + list(lint_msg, line_number = 9L, column_number = 1L) ), linters = linter ) From 9c99491be9585ef4d4ca107b51b9fd82cd78ab0d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:07:28 +0000 Subject: [PATCH 18/30] fixes --- R/fixed_regex_linter.R | 2 +- tests/testthat/test-class_equals_linter.R | 8 ++++---- tests/testthat/test-is_numeric_linter.R | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/R/fixed_regex_linter.R b/R/fixed_regex_linter.R index 46f4a57b59..5c456683cc 100644 --- a/R/fixed_regex_linter.R +++ b/R/fixed_regex_linter.R @@ -154,7 +154,7 @@ fixed_regex_linter <- function(allow_unescaped = FALSE) { call_name <- xml_find_chr(patterns, "string(preceding-sibling::expr[last()]/SYMBOL_FUNCTION_CALL)") is_stringr <- startsWith(call_name, "str_") - replace_suggestion <- ifelse( + replacement_suggestion <- ifelse( is_stringr, sprintf("stringr::fixed(%s) as the pattern", fixed_equivalent), sprintf("%s with fixed = TRUE", fixed_equivalent) diff --git a/tests/testthat/test-class_equals_linter.R b/tests/testthat/test-class_equals_linter.R index d406e4a760..9487a4c6cb 100644 --- a/tests/testthat/test-class_equals_linter.R +++ b/tests/testthat/test-class_equals_linter.R @@ -11,7 +11,7 @@ test_that("class_equals_linter skips allowed usages", { test_that("class_equals_linter blocks simple disallowed usages", { linter <- class_equals_linter() - lint_msg <- rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')") + lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')") expect_lint("if (class(x) == 'character') stop('no')", lint_msg, linter) expect_lint("is_regression <- class(x) == 'lm'", lint_msg, linter) @@ -20,7 +20,7 @@ test_that("class_equals_linter blocks simple disallowed usages", { test_that("class_equals_linter blocks usage of %in% for checking class", { linter <- class_equals_linter() - lint_msg <- rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')") + lint_msg <- rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')") expect_lint("if ('character' %in% class(x)) stop('no')", lint_msg, linter) expect_lint("if (class(x) %in% 'character') stop('no')", lint_msg, linter) @@ -29,7 +29,7 @@ test_that("class_equals_linter blocks usage of %in% for checking class", { test_that("class_equals_linter blocks class(x) != 'klass'", { expect_lint( "if (class(x) != 'character') TRUE", - rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')"), + rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')"), class_equals_linter() ) }) @@ -43,7 +43,7 @@ test_that("class_equals_linter skips usage for subsetting", { # but not further nesting expect_lint( "x[if (class(x) == 'foo') 1 else 2]", - rex::rex("Use inherits(x, 'class-name') or is. or is(x, 'class')"), + rex::rex("Use inherits(x, 'class-name'), is. or is(x, 'class')"), linter ) }) diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index c3278a07f1..ab235663c1 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -20,7 +20,7 @@ test_that("is_numeric_linter skips allowed usages involving %in%", { test_that("is_numeric_linter blocks disallowed usages involving ||", { linter <- is_numeric_linter() - lint_msg <- rex::rex("Use `is.numeric(x)` instead of equivalent `is.numeric(x) || is.integer(x)`.") + lint_msg <- rex::rex("Use `is.numeric(x)` instead of the equivalent `is.numeric(x) || is.integer(x)`.") expect_lint("is.numeric(x) || is.integer(x)", lint_msg, linter) @@ -48,7 +48,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { test_that("is_numeric_linter blocks disallowed usages involving %in%", { linter <- is_numeric_linter() - lint_msg <- rex::rex('Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c('integer', 'numeric')", lint_msg, linter) expect_lint('class(x) %in% c("numeric", "integer")', lint_msg, linter) @@ -58,7 +58,7 @@ test_that("raw strings are handled properly when testing in class", { skip_if_not_r_version("4.0.0") linter <- is_numeric_linter() - lint_msg <- rex::rex('Use is.numeric(x) instead of equivalent class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c(R'(numeric)', 'integer', 'factor')", NULL, linter) expect_lint("class(x) %in% c('numeric', R'--(integer)--', y)", NULL, linter) From 27ef7b516fe0855f8ae807f3109829aeb6782966 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:17:50 +0000 Subject: [PATCH 19/30] condense, set patrick dependency --- DESCRIPTION | 2 +- tests/testthat/test-nested_ifelse_linter.R | 47 ++++++---------------- 2 files changed, 13 insertions(+), 36 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 48c2b13b38..93160e755c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -40,7 +40,7 @@ Suggests: httr (>= 1.2.1), jsonlite, mockery, - patrick, + patrick (>= 0.2.0), rlang, rmarkdown, rstudioapi (>= 0.2), diff --git a/tests/testthat/test-nested_ifelse_linter.R b/tests/testthat/test-nested_ifelse_linter.R index a7bd0df687..2305e02555 100644 --- a/tests/testthat/test-nested_ifelse_linter.R +++ b/tests/testthat/test-nested_ifelse_linter.R @@ -11,47 +11,24 @@ test_that("nested_ifelse_linter skips allowed usages", { expect_lint("case_when(l1 ~ v1, l2 ~ v2)", NULL, linter) }) -test_that("nested_ifelse_linter blocks simple disallowed usages", { - expect_lint( - "ifelse(l1, v1, ifelse(l2, v2, v3))", - rex::rex("Avoid nested ifelse() calls"), - nested_ifelse_linter() - ) +local({ + ifelse_calls <- c("ifelse", "if_else", "fifelse") - expect_lint( - "ifelse(l1, ifelse(l2, v1, v2), v3)", - rex::rex("Avoid nested ifelse() calls"), - nested_ifelse_linter() - ) -}) + linter <- nested_ifelse_linter() -test_that("nested_ifelse_linter also catches dplyr::if_else", { - expect_lint( - "if_else(l1, v1, if_else(l2, v2, v3))", - rex::rex("Avoid nested if_else() calls"), - nested_ifelse_linter() - ) + patrick::with_parameters_test_that( + "nested_ifelse_linter blocks simple disallowed usages", + { + lint_msg <- rex::rex("Avoid nested ", ifelse_call, " calls") - expect_lint( - "dplyr::if_else(l1, dplyr::if_else(l2, v1, v2), v3)", - rex::rex("Avoid nested if_else() calls"), - nested_ifelse_linter() + expect_lint(glue::glue("{ifelse_call}(l1, v1, {ifelse_call}(l2, v2, v3))"), lint_msg, linter) + expect_lint(glue::glue("{ifelse_call}(l1, {ifelse_call}(l2, v1, v2), v3)"), lint_msg, linter) + }, + ifelse_call = ifelse_calls ) }) -test_that("nested_ifelse_linter also catches data.table::fifelse", { - expect_lint( - "fifelse(l1, v1, fifelse(l2, v2, v3))", - rex::rex("Avoid nested fifelse() calls"), - nested_ifelse_linter() - ) - - expect_lint( - "data.table::fifelse(l1, v1, data.table::fifelse(l2, v2, v3))", - rex::rex("Avoid nested fifelse() calls"), - nested_ifelse_linter() - ) - +test_that("nested_ifelse_linter also catches mixed usage", { # not sure why anyone would do this, but the readability still argument holds expect_lint( "data.table::fifelse(l1, dplyr::if_else(l2, v1, v2), v3)", From d53644e8d6ec2459132ef04a769047c582d62acc Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:19:47 +0000 Subject: [PATCH 20/30] consistency in file name --- ...t-pipe-consistency-linter.R => test-pipe_consistency_linter.R} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/testthat/{test-pipe-consistency-linter.R => test-pipe_consistency_linter.R} (100%) diff --git a/tests/testthat/test-pipe-consistency-linter.R b/tests/testthat/test-pipe_consistency_linter.R similarity index 100% rename from tests/testthat/test-pipe-consistency-linter.R rename to tests/testthat/test-pipe_consistency_linter.R From 18cca03e4d894f6ae0647ef31260aa0d8df41b68 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:22:49 +0000 Subject: [PATCH 21/30] checks= is required here --- tests/testthat/test-trailing_blank_lines_linter.R | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testthat/test-trailing_blank_lines_linter.R b/tests/testthat/test-trailing_blank_lines_linter.R index 1775d78169..5b6f89511b 100644 --- a/tests/testthat/test-trailing_blank_lines_linter.R +++ b/tests/testthat/test-trailing_blank_lines_linter.R @@ -58,7 +58,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp3, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - list(lint_msg, line_number = 10L, column_number = 1L), + checks = list(lint_msg, line_number = 10L, column_number = 1L), linters = linter ) @@ -77,7 +77,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp4, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - list(lint_msg, line_number = 5L, column_number = 1L), + checks = list(lint_msg, line_number = 5L, column_number = 1L), linters = linter ) @@ -101,7 +101,7 @@ test_that("trailing_blank_lines_linter detects missing terminal newlines in Rmd/ expect_lint( file = tmp5, # We can't get 4 here because the line is NA-masked in get_source_expressions(), so no line length info exists. - list(lint_msg, line_number = 10L, column_number = 1L), + checks = list(lint_msg, line_number = 10L, column_number = 1L), linters = linter ) }) @@ -127,7 +127,7 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp6, - list(lint_msg, line_number = 7L, column_number = 1L), + checks = list(lint_msg, line_number = 7L, column_number = 1L), linters = linter ) @@ -150,7 +150,7 @@ test_that("blank lines in knitr chunks produce lints", { expect_lint( file = tmp7, - list( + checks = list( list(lint_msg, line_number = 7L, column_number = 1L), list(lint_msg, line_number = 8L, column_number = 1L), list(lint_msg, line_number = 9L, column_number = 1L) From cc273489164c20b7e4dcb910417ef0d7e9135f2d Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:27:22 +0000 Subject: [PATCH 22/30] fix lint message --- tests/testthat/test-nested_ifelse_linter.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-nested_ifelse_linter.R b/tests/testthat/test-nested_ifelse_linter.R index 2305e02555..808d83278f 100644 --- a/tests/testthat/test-nested_ifelse_linter.R +++ b/tests/testthat/test-nested_ifelse_linter.R @@ -19,7 +19,7 @@ local({ patrick::with_parameters_test_that( "nested_ifelse_linter blocks simple disallowed usages", { - lint_msg <- rex::rex("Avoid nested ", ifelse_call, " calls") + lint_msg <- rex::rex("Avoid nested ", ifelse_call, "() calls") expect_lint(glue::glue("{ifelse_call}(l1, v1, {ifelse_call}(l2, v2, v3))"), lint_msg, linter) expect_lint(glue::glue("{ifelse_call}(l1, {ifelse_call}(l2, v1, v2), v3)"), lint_msg, linter) From 8961e37de1b380febc6fb0853212b89fd6af1879 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 5 Dec 2023 02:33:26 +0000 Subject: [PATCH 23/30] condense --- tests/testthat/test-seq_linter.R | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/testthat/test-seq_linter.R b/tests/testthat/test-seq_linter.R index 7edaee13e8..d63ad2430b 100644 --- a/tests/testthat/test-seq_linter.R +++ b/tests/testthat/test-seq_linter.R @@ -1,43 +1,43 @@ test_that("other : expressions are fine", { linter <- seq_linter() - expect_lint("function() { 1:10 }", NULL, linter) - expect_lint("function(x) { 2:length(x) }", NULL, linter) - expect_lint("function(x) { 1:(length(x) || 1) }", NULL, linter) + expect_lint("1:10", NULL, linter) + expect_lint("2:length(x)", NULL, linter) + expect_lint("1:(length(x) || 1)", NULL, linter) }) test_that("seq_len(...) or seq_along(...) expressions are fine", { linter <- seq_linter() - expect_lint("function(x) { seq_len(x) }", NULL, linter) - expect_lint("function(x) { seq_along(x) }", NULL, linter) + expect_lint("seq_len(x)", NULL, linter) + expect_lint("seq_along(x)", NULL, linter) - expect_lint("function(x) { seq(2, length(x)) }", NULL, linter) - expect_lint("function(x) { seq(length(x), 2) }", NULL, linter) + expect_lint("seq(2, length(x))", NULL, linter) + expect_lint("seq(length(x), 2)", NULL, linter) }) test_that("finds seq(...) expressions", { linter <- seq_linter() expect_lint( - "function(x) { seq(length(x)) }", + "seq(length(x))", rex::rex("Use seq_along(...) instead of seq(length(...))"), linter ) expect_lint( - "function(x) { seq(nrow(x)) }", + "seq(nrow(x))", rex::rex("Use seq_len(nrow(...)) instead of seq(nrow(...))"), linter ) expect_lint( - "function(x) { rev(seq(length(x))) }", + "rev(seq(length(x)))", rex::rex("Use seq_along(...) instead of seq(length(...))"), linter ) expect_lint( - "function(x) { rev(seq(nrow(x))) }", + "rev(seq(nrow(x)))", rex::rex("Use seq_len(nrow(...)) instead of seq(nrow(...))"), linter ) From ed21a0881237fe652c393697df6358f253ac9a0b Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 09:18:46 +0800 Subject: [PATCH 24/30] review changes --- R/duplicate_argument_linter.R | 2 +- R/namespace_linter.R | 2 +- tests/testthat/test-duplicate_argument_linter.R | 4 ++-- tests/testthat/test-namespace_linter.R | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/R/duplicate_argument_linter.R b/R/duplicate_argument_linter.R index 883d16faeb..ca579b82e7 100644 --- a/R/duplicate_argument_linter.R +++ b/R/duplicate_argument_linter.R @@ -58,7 +58,7 @@ duplicate_argument_linter <- function(except = c("mutate", "transmute")) { xml_nodes_to_lints( unlist(all_arg_nodes, recursive = FALSE)[unlist(is_duplicated)], source_expression = source_expression, - lint_message = "Remove duplicate arguments in function call.", + lint_message = "Avoid duplicate arguments in function calls.", type = "warning" ) }) diff --git a/R/namespace_linter.R b/R/namespace_linter.R index 24fd465029..d709e6b383 100644 --- a/R/namespace_linter.R +++ b/R/namespace_linter.R @@ -157,7 +157,7 @@ build_ns_get_int_lints <- function(packages, symbols, symbol_nodes, namespaces, symbol_nodes[exported], source_expression = source_expression, lint_message = - sprintf("Use %2$s::%1$s, not %2$s:::%1$s.", symbols[exported], packages[exported]), + sprintf("Don't use `:::` to access %s, which is exported from %s.", symbols[exported], packages[exported]), type = "warning" ) diff --git a/tests/testthat/test-duplicate_argument_linter.R b/tests/testthat/test-duplicate_argument_linter.R index 2ddd6da922..05a5e51cb8 100644 --- a/tests/testthat/test-duplicate_argument_linter.R +++ b/tests/testthat/test-duplicate_argument_linter.R @@ -11,7 +11,7 @@ test_that("duplicate_argument_linter doesn't block allowed usages", { test_that("duplicate_argument_linter blocks disallowed usages", { linter <- duplicate_argument_linter() - lint_msg <- rex::rex("Remove duplicate arguments in function call.") + lint_msg <- rex::rex("Avoid duplicate arguments in function calls.") expect_lint("fun(arg = 1, arg = 2)", lint_msg, linter) expect_lint("fun(arg = 1, 'arg' = 2)", lint_msg, linter) @@ -51,7 +51,7 @@ test_that("duplicate_argument_linter respects except argument", { "fun(` ` = 1, ` ` = 2)", - rex::rex("Remove duplicate arguments in function call."), + rex::rex("Avoid duplicate arguments in function calls."), duplicate_argument_linter(except = character()) ) diff --git a/tests/testthat/test-namespace_linter.R b/tests/testthat/test-namespace_linter.R index d311cc74de..9eb45cdc76 100644 --- a/tests/testthat/test-namespace_linter.R +++ b/tests/testthat/test-namespace_linter.R @@ -52,7 +52,7 @@ test_that("namespace_linter blocks disallowed usages", { expect_lint( "stats:::sd(c(1,2,3))", - rex::rex("Use stats::sd, not stats:::sd"), + rex::rex("Don't use `:::` to access sd, which is exported from stats."), linter ) From 57eeaa5fb1f48b990956e2e27c1518c4d8a32fbb Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 01:19:32 +0000 Subject: [PATCH 25/30] revert --- R/nested_pipe_linter.R | 2 +- tests/testthat/test-nested_pipe_linter.R | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/R/nested_pipe_linter.R b/R/nested_pipe_linter.R index 46190e0805..a7a8c323b7 100644 --- a/R/nested_pipe_linter.R +++ b/R/nested_pipe_linter.R @@ -76,7 +76,7 @@ nested_pipe_linter <- function( xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = "Avoid nesting pipes inside other calls.", + lint_message = "Don't nest pipes inside other calls.", type = "warning" ) }) diff --git a/tests/testthat/test-nested_pipe_linter.R b/tests/testthat/test-nested_pipe_linter.R index 3072d3ced3..1e16792385 100644 --- a/tests/testthat/test-nested_pipe_linter.R +++ b/tests/testthat/test-nested_pipe_linter.R @@ -58,7 +58,7 @@ patrick::with_parameters_test_that( test_that("nested_pipe_linter blocks simple disallowed usages", { linter <- nested_pipe_linter() linter_inline <- nested_pipe_linter(allow_inline = FALSE) - lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") + lint_msg <- rex::rex("Don't nest pipes inside other calls.") expect_lint( "bind_rows(a %>% select(b), c %>% select(b))", @@ -110,7 +110,7 @@ test_that("allow_outer_calls= argument works", { foo() ) "), - rex::rex("Avoid nesting pipes inside other calls."), + rex::rex("Don't nest pipes inside other calls."), nested_pipe_linter(allow_outer_calls = character()) ) @@ -131,7 +131,7 @@ test_that("Native pipes are handled as well", { linter <- nested_pipe_linter() linter_inline <- nested_pipe_linter(allow_inline = FALSE) - lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") + lint_msg <- rex::rex("Don't nest pipes inside other calls.") expect_lint( "bind_rows(a |> select(b), c |> select(b))", @@ -157,7 +157,7 @@ test_that("Native pipes are handled as well", { }) test_that("lints vectorize", { - lint_msg <- rex::rex("Avoid nesting pipes inside other calls.") + lint_msg <- rex::rex("Don't nest pipes inside other calls.") lines <- trim_some("{ bind_rows( From b91f65fa1e7c9955f28f69999b23675b600cd32e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 01:22:21 +0000 Subject: [PATCH 26/30] commas feedback --- R/commas_linter.R | 4 ++-- tests/testthat/test-commas_linter.R | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/R/commas_linter.R b/R/commas_linter.R index 407278bd7d..af4226cee1 100644 --- a/R/commas_linter.R +++ b/R/commas_linter.R @@ -84,7 +84,7 @@ commas_linter <- function(allow_trailing = FALSE) { before_lints <- xml_nodes_to_lints( xml_find_all(xml, xpath_before), source_expression = source_expression, - lint_message = "Commas should never follow a space.", + lint_message = "Remove spaces before a comma.", range_start_xpath = "number(./preceding-sibling::*[1]/@col2 + 1)", # start after preceding expression range_end_xpath = "number(./@col1 - 1)" # end before comma ) @@ -92,7 +92,7 @@ commas_linter <- function(allow_trailing = FALSE) { after_lints <- xml_nodes_to_lints( xml_find_all(xml, xpath_after), source_expression = source_expression, - lint_message = "Commas should always precede a space.", + lint_message = "Put a space after a comma.", range_start_xpath = "number(./@col2 + 1)", # start and end after comma range_end_xpath = "number(./@col2 + 1)" ) diff --git a/tests/testthat/test-commas_linter.R b/tests/testthat/test-commas_linter.R index 05e853a2fa..423a32cc90 100644 --- a/tests/testthat/test-commas_linter.R +++ b/tests/testthat/test-commas_linter.R @@ -1,7 +1,7 @@ test_that("returns the correct linting (with default parameters)", { linter <- commas_linter() - msg_after <- rex::rex("Commas should always precede a space.") - msg_before <- rex::rex("Commas should never follow a space.") + msg_after <- rex::rex("Put a space after a comma.") + msg_before <- rex::rex("Remove spaces before a comma.") expect_lint("blah", NULL, linter) expect_lint("fun(1, 1)", NULL, linter) @@ -52,8 +52,8 @@ test_that("returns the correct linting (with default parameters)", { test_that("returns the correct linting (with 'allow_trailing' set)", { linter <- commas_linter(allow_trailing = TRUE) - msg_after <- rex::rex("Commas should always precede a space.") - msg_before <- rex::rex("Commas should never follow a space.") + msg_after <- rex::rex("Put a space after a comma.") + msg_before <- rex::rex("Remove spaces before a comma.") expect_lint("blah", NULL, linter) expect_lint("fun(1, 1)", NULL, linter) From aaa7268a7f920159eba5e6a70f822d10c651d622 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 01:24:06 +0000 Subject: [PATCH 27/30] implicit integers feedback --- R/implicit_integer_linter.R | 3 +-- tests/testthat/test-implicit_integer_linter.R | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/R/implicit_integer_linter.R b/R/implicit_integer_linter.R index ea3a4bbebc..5dae749a86 100644 --- a/R/implicit_integer_linter.R +++ b/R/implicit_integer_linter.R @@ -60,8 +60,7 @@ implicit_integer_linter <- function(allow_colon = FALSE) { xml_nodes_to_lints( numbers[is_implicit_integer(xml_text(numbers))], source_expression = source_expression, - lint_message = - "Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.", + lint_message = "Avoid implicit integers. Use e.g. 1L for integers or 1.0 for doubles.", type = "style", column_number_xpath = "number(./@col2 + 1)", # mark at end range_end_xpath = "number(./@col2 + 1)" # end after number for easy fixing (enter "L" or ".0") diff --git a/tests/testthat/test-implicit_integer_linter.R b/tests/testthat/test-implicit_integer_linter.R index cddeff0447..1aa57d146d 100644 --- a/tests/testthat/test-implicit_integer_linter.R +++ b/tests/testthat/test-implicit_integer_linter.R @@ -56,8 +56,7 @@ local({ test_that("linter returns the correct linting", { linter <- implicit_integer_linter() - lint_msg <- - rex::rex("Avoid implicit integers. Make the type explicit by using the form 1L for integers or 1.0 for doubles.") + lint_msg <- rex::rex("Avoid implicit integers. Use e.g. 1L for integers or 1.0 for doubles.") expect_lint("x <<- 1L", NULL, linter) expect_lint("1.0/-Inf -> y", NULL, linter) From f63cd18984f194dd5a164dc10227ce4b4744b6c6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 01:28:03 +0000 Subject: [PATCH 28/30] one more revert --- R/nested_ifelse_linter.R | 4 +- tests/testthat/test-nested_ifelse_linter.R | 49 ++++++++++++++++------ 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/R/nested_ifelse_linter.R b/R/nested_ifelse_linter.R index 68d1d4905c..8657333ff9 100644 --- a/R/nested_ifelse_linter.R +++ b/R/nested_ifelse_linter.R @@ -94,8 +94,8 @@ nested_ifelse_linter <- function() { matched_call <- xp_call_name(bad_expr) lint_message <- paste( - sprintf("Avoid nested %s() calls.", matched_call), - "Instead, try (1) data.table::fcase(); (2) dplyr::case_when(); or (3) using a lookup table." + sprintf("Don't use nested %s() calls;", matched_call), + "instead, try (1) data.table::fcase; (2) dplyr::case_when; or (3) using a lookup table." ) xml_nodes_to_lints(bad_expr, source_expression, lint_message, type = "warning") }) diff --git a/tests/testthat/test-nested_ifelse_linter.R b/tests/testthat/test-nested_ifelse_linter.R index 808d83278f..14bd3302f5 100644 --- a/tests/testthat/test-nested_ifelse_linter.R +++ b/tests/testthat/test-nested_ifelse_linter.R @@ -11,28 +11,51 @@ test_that("nested_ifelse_linter skips allowed usages", { expect_lint("case_when(l1 ~ v1, l2 ~ v2)", NULL, linter) }) -local({ - ifelse_calls <- c("ifelse", "if_else", "fifelse") +test_that("nested_ifelse_linter blocks simple disallowed usages", { + expect_lint( + "ifelse(l1, v1, ifelse(l2, v2, v3))", + rex::rex("Don't use nested ifelse() calls"), + nested_ifelse_linter() + ) - linter <- nested_ifelse_linter() + expect_lint( + "ifelse(l1, ifelse(l2, v1, v2), v3)", + rex::rex("Don't use nested ifelse() calls"), + nested_ifelse_linter() + ) +}) - patrick::with_parameters_test_that( - "nested_ifelse_linter blocks simple disallowed usages", - { - lint_msg <- rex::rex("Avoid nested ", ifelse_call, "() calls") +test_that("nested_ifelse_linter also catches dplyr::if_else", { + expect_lint( + "if_else(l1, v1, if_else(l2, v2, v3))", + rex::rex("Don't use nested if_else() calls"), + nested_ifelse_linter() + ) - expect_lint(glue::glue("{ifelse_call}(l1, v1, {ifelse_call}(l2, v2, v3))"), lint_msg, linter) - expect_lint(glue::glue("{ifelse_call}(l1, {ifelse_call}(l2, v1, v2), v3)"), lint_msg, linter) - }, - ifelse_call = ifelse_calls + expect_lint( + "dplyr::if_else(l1, dplyr::if_else(l2, v1, v2), v3)", + rex::rex("Don't use nested if_else() calls"), + nested_ifelse_linter() ) }) -test_that("nested_ifelse_linter also catches mixed usage", { +test_that("nested_ifelse_linter also catches data.table::fifelse", { + expect_lint( + "fifelse(l1, v1, fifelse(l2, v2, v3))", + rex::rex("Don't use nested fifelse() calls"), + nested_ifelse_linter() + ) + + expect_lint( + "data.table::fifelse(l1, v1, data.table::fifelse(l2, v2, v3))", + rex::rex("Don't use nested fifelse() calls"), + nested_ifelse_linter() + ) + # not sure why anyone would do this, but the readability still argument holds expect_lint( "data.table::fifelse(l1, dplyr::if_else(l2, v1, v2), v3)", - rex::rex("Avoid nested if_else() calls"), + rex::rex("Don't use nested if_else() calls"), nested_ifelse_linter() ) }) From fc612f8b211475ef892650afc22244c21fcd4f59 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 05:35:18 +0000 Subject: [PATCH 29/30] drop "the equivalent" --- R/is_numeric_linter.R | 2 +- tests/testthat/test-is_numeric_linter.R | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/is_numeric_linter.R b/R/is_numeric_linter.R index 2ca29871c8..c9d22103d1 100644 --- a/R/is_numeric_linter.R +++ b/R/is_numeric_linter.R @@ -97,7 +97,7 @@ is_numeric_linter <- function() { class_expr, source_expression = source_expression, lint_message = paste( - 'Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric").', + 'Use is.numeric(x) instead of class(x) %in% c("integer", "numeric").', "Use is.double(x) to test for objects stored as 64-bit floating point." ), type = "warning" diff --git a/tests/testthat/test-is_numeric_linter.R b/tests/testthat/test-is_numeric_linter.R index ab235663c1..e5c8faacba 100644 --- a/tests/testthat/test-is_numeric_linter.R +++ b/tests/testthat/test-is_numeric_linter.R @@ -48,7 +48,7 @@ test_that("is_numeric_linter blocks disallowed usages involving ||", { test_that("is_numeric_linter blocks disallowed usages involving %in%", { linter <- is_numeric_linter() - lint_msg <- rex::rex('Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c('integer', 'numeric')", lint_msg, linter) expect_lint('class(x) %in% c("numeric", "integer")', lint_msg, linter) @@ -58,7 +58,7 @@ test_that("raw strings are handled properly when testing in class", { skip_if_not_r_version("4.0.0") linter <- is_numeric_linter() - lint_msg <- rex::rex('Use is.numeric(x) instead of the equivalent class(x) %in% c("integer", "numeric")') + lint_msg <- rex::rex('Use is.numeric(x) instead of class(x) %in% c("integer", "numeric")') expect_lint("class(x) %in% c(R'(numeric)', 'integer', 'factor')", NULL, linter) expect_lint("class(x) %in% c('numeric', R'--(integer)--', y)", NULL, linter) From c5d8fc064aa4338e753d9b7dcb6fc1d91a5fce49 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 6 Dec 2023 07:09:26 +0000 Subject: [PATCH 30/30] restore after fix already in main --- R/vector_logic_linter.R | 3 ++- tests/testthat/test-vector_logic_linter.R | 29 ++++++++++++++--------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/R/vector_logic_linter.R b/R/vector_logic_linter.R index c0c961b9c4..1cac35b1f4 100644 --- a/R/vector_logic_linter.R +++ b/R/vector_logic_linter.R @@ -86,10 +86,11 @@ vector_logic_linter <- function() { if (is.null(xml)) return(list()) bad_expr <- xml_find_all(xml, xpath) + op <- xml_text(bad_expr) xml_nodes_to_lints( bad_expr, source_expression = source_expression, - lint_message = "Use scalar logical operators (&& and ||) in conditional expressions.", + lint_message = sprintf("Use `%s` in conditional expressions.", strrep(op, 2L)), type = "warning" ) }) diff --git a/tests/testthat/test-vector_logic_linter.R b/tests/testthat/test-vector_logic_linter.R index 01bc79aef6..38b98b7031 100644 --- a/tests/testthat/test-vector_logic_linter.R +++ b/tests/testthat/test-vector_logic_linter.R @@ -29,30 +29,37 @@ test_that("vector_logic_linter skips allowed usages", { test_that("vector_logic_linter blocks simple disallowed usages", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") - expect_lint("if (TRUE & FALSE) 1", lint_msg, linter) - expect_lint("while (TRUE | TRUE) 2", lint_msg, linter) + expect_lint("if (TRUE & FALSE) 1", rex::rex("Use `&&` in conditional expressions."), linter) + expect_lint("while (TRUE | TRUE) 2", rex::rex("Use `||` in conditional expressions."), linter) }) test_that("vector_logic_linter detects nested conditions", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") - expect_lint("if (TRUE & TRUE || FALSE) 4", lint_msg, linter) - expect_lint("if (TRUE && (TRUE | FALSE)) 4", lint_msg, linter) + expect_lint( + "if (TRUE & TRUE || FALSE) 4", + list(rex::rex("Use `&&` in conditional expressions."), column_number = 10L), + linter + ) + expect_lint( + "if (TRUE && (TRUE | FALSE)) 4", + list(rex::rex("Use `||` in conditional expressions."), column_number = 19L), + linter + ) }) test_that("vector_logic_linter catches usages in expect_true()/expect_false()", { linter <- vector_logic_linter() - lint_msg <- rex::rex("Use scalar logical operators (&& and ||) in conditional expressions.") + and_msg <- rex::rex("Use `&&` in conditional expressions.") + or_msg <- rex::rex("Use `||` in conditional expressions.") - expect_lint("expect_true(TRUE & FALSE)", lint_msg, linter) - expect_lint("expect_false(TRUE | TRUE)", lint_msg, linter) + expect_lint("expect_true(TRUE & FALSE)", and_msg, linter) + expect_lint("expect_false(TRUE | TRUE)", or_msg, linter) # ditto with namespace qualification - expect_lint("testthat::expect_true(TRUE & FALSE)", lint_msg, linter) - expect_lint("testthat::expect_false(TRUE | TRUE)", lint_msg, linter) + expect_lint("testthat::expect_true(TRUE & FALSE)", and_msg, linter) + expect_lint("testthat::expect_false(TRUE | TRUE)", or_msg, linter) }) test_that("vector_logic_linter doesn't get mixed up from complex usage", {