Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New linter for complex conditional expressions #2676

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0417757
initial draft
IndrajeetPatil Oct 19, 2024
031329f
docs
IndrajeetPatil Oct 19, 2024
bd616ad
Create test-complex-conditional-linter.R
IndrajeetPatil Oct 19, 2024
f402853
fix XPath
IndrajeetPatil Oct 26, 2024
1acf920
simplify xpath
IndrajeetPatil Oct 26, 2024
575db33
fix failing tests
IndrajeetPatil Oct 26, 2024
78b0130
accept numeric values
IndrajeetPatil Oct 26, 2024
0269a5f
fix examples
IndrajeetPatil Oct 26, 2024
c6929fd
Update test-unnecessary_nesting_linter.R
IndrajeetPatil Oct 26, 2024
49acffd
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Oct 26, 2024
66c8102
Update NEWS.md
IndrajeetPatil Oct 30, 2024
746eba2
change default and check for input edge cases
IndrajeetPatil Nov 1, 2024
bea5900
Update test-complex-conditional-linter.R
IndrajeetPatil Nov 1, 2024
fed5efa
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Nov 1, 2024
249db6e
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Nov 18, 2024
16896a0
clean the new lints
IndrajeetPatil Nov 21, 2024
f6a5b61
base and internal copies not the same
IndrajeetPatil Nov 21, 2024
a685c99
fix tests
IndrajeetPatil Nov 21, 2024
532b9c2
rename test file
IndrajeetPatil Nov 24, 2024
421c1db
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Nov 27, 2024
c04e2f6
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Feb 3, 2025
3bb5f63
update for patrick
IndrajeetPatil Feb 3, 2025
8de0c31
Merge branch 'main' into f1830-complex-conditional-linter
IndrajeetPatil Feb 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update NEWS.md
IndrajeetPatil committed Oct 30, 2024
commit 66c810271ca9304813188aed1460b36d487edf0a
4 changes: 2 additions & 2 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -19,7 +19,6 @@
* `scalar_in_linter` is now configurable to allow other `%in%` like operators to be linted. The data.table operator `%chin%` is no longer linted by default; use `in_operators = "%chin%"` to continue linting it. (@F-Noelle)
* `lint()` and friends now normalize paths to forward slashes on Windows (@olivroy, #2613).
* `undesirable_function_linter()`, `undesirable_operator_linter()`, and `list_comparison_linter()` were removed from the tag `efficiency` (@IndrajeetPatil, #2655). If you use `linters_with_tags("efficiency")` to include these linters, you'll need to adjust your config to keep linting your code against them. We did not find any such users on GitHub.


## Bug fixes

@@ -47,7 +46,7 @@
+ `return_functions` to customize which functions are equivalent to `return()` as "exit" clauses, e.g. `rlang::abort()` can be considered in addition to the default functions like `stop()` and `q()` from base (#2271 and part of #884, @MichaelChirico and @MEO265).
+ `except` to customize which functions are ignored entirely (i.e., whether they have a return of the specified style is not checked; #2271 and part of #884, @MichaelChirico and @MEO265). Namespace hooks like `.onAttach()` and `.onLoad()` are always ignored.
+ `except_regex`, the same purpose as `except=`, but filters functions by pattern. This is motivated by {RUnit}, where test suites are based on unit test functions matched by pattern, e.g. `^Test`, and where explicit return may be awkward (#2335, @MichaelChirico).
* `unnecessary_lambda_linter` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`.
* `unnecessary_lambda_linter()` is extended to encourage vectorized comparisons where possible, e.g. `sapply(x, sum) > 0` instead of `sapply(x, function(x) sum(x) > 0)` (part of #884, @MichaelChirico). Toggle this behavior with argument `allow_comparison`.
* `backport_linter()` is slightly faster by moving expensive computations outside the linting function (#2339, #2348, @AshesITR and @MichaelChirico).
* `Linter()` has a new argument `linter_level` (default `NA`). This is used by `lint()` to more efficiently check for expression levels than the idiom `if (!is_lint_level(...)) { return(list()) }` (#2351, @AshesITR).
* `string_boundary_linter()` recognizes regular expression calls like `grepl("^abc$", x)` that can be replaced by using `==` instead (#1613, @MichaelChirico).
@@ -83,6 +82,7 @@
* `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico).
* `one_call_pipe_linter()` for discouraging one-step pipelines like `x |> as.character()` (#2330 and part of #884, @MichaelChirico).
* `object_overwrite_linter()` for discouraging re-use of upstream package exports as local variables (#2344, #2346 and part of #884, @MichaelChirico and @AshesITR).
* `complex_conditional_linter()` for encouraging refactoring of complex conditional expressions (like `if (x > 0 && y < 0 || z == 0)`) via well-named abstractions (#2676, @IndrajeetPatil).

### Lint accuracy fixes: removing false positives


Unchanged files with check annotations Beta

source_expression$content <- get_content(source_expression$lines)
parsed_content <- get_source_expression(source_expression, error = function(e) lint_parse_error(e, source_expression))
if (inherits(e, "lint") && (is.na(e$line) || !nzchar(e$line) || e$message == "unexpected end of input")) {

Check warning on line 89 in R/get_source_expressions.R

GitHub Actions / lint

file=R/get_source_expressions.R,line=89,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
# Don't create expression list if it's unreliable (invalid encoding or unhandled parse error)
expressions <- list()
} else {
# A linter factory is a function whose last call is to Linter()
bdexpr <- body(fun)
# covr internally transforms each call into if (TRUE) { covr::count(...); call }
while (is.call(bdexpr) && (bdexpr[[1L]] == "{" || (bdexpr[[1L]] == "if" && bdexpr[[2L]] == "TRUE"))) {

Check warning on line 367 in R/lint.R

GitHub Actions / lint

file=R/lint.R,line=367,col=10,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
bdexpr <- bdexpr[[length(bdexpr)]]
}
is.call(bdexpr) && identical(bdexpr[[1L]], as.name("Linter"))
cli_abort("{.arg line} must be a string.", call. = FALSE)
}
max_col <- max(nchar(line) + 1L, 1L, na.rm = TRUE)
if (!is_number(column_number) || column_number < 0L || column_number > max_col) {

Check warning on line 413 in R/lint.R

GitHub Actions / lint

file=R/lint.R,line=413,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
cli_abort("
{.arg column_number} must be an integer between {.val {0}} and {.val {max_col}} ({.code nchar(line) + 1}),
not {.obj_type_friendly {column_number}}.
#' @seealso [linters] for a complete list of linters available in lintr.
#' @export
object_name_linter <- function(styles = c("snake_case", "symbols"), regexes = character()) {
if ((!missing(styles) || missing(regexes)) && length(styles) > 0L) {

Check warning on line 86 in R/object_name_linter.R

GitHub Actions / lint

file=R/object_name_linter.R,line=86,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
# Allow `object_name_linter(NULL, "my_regex")`
styles <- match.arg(styles, names(style_regexes), several.ok = TRUE)
style_list <- style_regexes[styles]
if (!is.character(path)) {
cli_abort("Argument {.arg path} should be a {.cls character} vector.")
}
if (!is.character(sep) || length(sep) != 1L || !nzchar(sep)) {

Check warning on line 113 in R/path_utils.R

GitHub Actions / lint

file=R/path_utils.R,line=113,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
cli_abort("Argument {.arg sep} should be a non-empty regular expression character string.")
}
Map(split_path, strsplit(path, sep), substr(path, 1L, 1L))
n_magrittr <- length(match_magrittr)
n_native <- length(match_native)
if (pipe == "auto" && n_magrittr > 0L && n_native > 0L) {

Check warning on line 50 in R/pipe_consistency_linter.R

GitHub Actions / lint

file=R/pipe_consistency_linter.R,line=50,col=9,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
xml_nodes_to_lints(
xml = c(match_magrittr, match_native),
source_expression = source_expression,
line_number <- length(source_expression$file_lines)
lints <- list()
while (line_number > 0L && (is.na(blanks[[line_number]]) || isTRUE(blanks[[line_number]]))) {

Check warning on line 37 in R/trailing_blank_lines_linter.R

GitHub Actions / lint

file=R/trailing_blank_lines_linter.R,line=37,col=12,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
if (!is.na(blanks[[line_number]])) {
lints[[length(lints) + 1L]] <- Lint(
filename = source_expression$filename,
undesirable_function_linter <- function(fun = default_undesirable_functions,
symbol_is_undesirable = TRUE) {
stopifnot(is.logical(symbol_is_undesirable))
if (is.null(names(fun)) || !all(nzchar(names(fun))) || length(fun) == 0L) {

Check warning on line 60 in R/undesirable_function_linter.R

GitHub Actions / lint

file=R/undesirable_function_linter.R,line=60,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
cli_abort(c(
x = "{.arg fun} should be a non-empty named character vector.",
i = "Use missing elements to indicate default messages."
#' @seealso [linters] for a complete list of linters available in lintr.
#' @export
undesirable_operator_linter <- function(op = default_undesirable_operators) {
if (is.null(names(op)) || !all(nzchar(names(op))) || length(op) == 0L) {

Check warning on line 46 in R/undesirable_operator_linter.R

GitHub Actions / lint

file=R/undesirable_operator_linter.R,line=46,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
cli_abort(c(
x = "{.arg op} should be a non-empty named character vector.",
i = "Use missing elements to indicate default messages."
`%||%` <- function(x, y) {
if (is.null(x) || length(x) == 0L || (is.atomic(x[[1L]]) && is.na(x[[1L]]))) {

Check warning on line 2 in R/utils.R

GitHub Actions / lint

file=R/utils.R,line=2,col=7,[complex_conditional_linter] Complex conditional with more than 1 logical operator(s). Consider extracting into a boolean function or variable for readability and reusability.
y
} else {
x