Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
* New argument `include_s4_slots` for the `xml_find_function_calls()` entry in the `get_source_expressions()` to govern whether calls of the form `s4Obj@fun()` are included in the result (#2820, @MichaelChirico).
* `sprintf_linter()` lints `sprintf()` and `gettextf()` calls when a constant string is passed to `fmt` (#2894, @Bisaloo).
* `use_lintr()` adds the created `.lintr` file to the `.Rbuildignore` if run in a package (#2926, initial work by @MEO265, finalized by @Bisaloo).
* `sort_linter()` recommends usage of `!is.unsorted(x)` over `identical(x, sort(x))` (#2921, @Bisaloo).

### New linters

Expand Down
24 changes: 22 additions & 2 deletions R/sort_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,13 @@ sort_linter <- function() {
and expr/expr = expr
]
"

sorted_identical_xpath <- "
parent::expr[not(SYMBOL_SUB)]
/parent::expr[
expr/SYMBOL_FUNCTION_CALL[text() = 'identical']
and expr/expr = expr
]
"

arguments_xpath <-
".//SYMBOL_SUB[text() = 'method' or text() = 'decreasing' or text() = 'na.last']"
Expand Down Expand Up @@ -142,8 +148,22 @@ sort_linter <- function() {
"Use is.unsorted(x) to test the unsortedness of a vector."
)

sorted_identical_expr <- xml_find_all(xml_calls, sorted_identical_xpath)
is_negated <- !is.na(
xml_find_first(sorted_identical_expr, "preceding-sibling::*[not(self::COMMENT)][1][self::OP-EXCLAMATION]")
)

lint_message <- c(
lint_message,
ifelse(
is_negated,
"Use is.unsorted(x) to test the unsortedness of a vector.",
"Use !is.unsorted(x) to test the sortedness of a vector."
)
)

sorted_lints <- xml_nodes_to_lints(
sorted_expr,
combine_nodesets(sorted_expr, sorted_identical_expr),
source_expression = source_expression,
lint_message = lint_message,
type = "warning"
Expand Down
4 changes: 4 additions & 0 deletions tests/testthat/test-sort_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,19 @@ test_that("sort_linter blocks simple disallowed usages", {
sorted_msg <- rex::rex("Use !is.unsorted(x) to test the sortedness of a vector.")

expect_lint("sort(x) == x", sorted_msg, linter)
expect_lint("identical(x, sort(x))", sorted_msg, linter)

# argument order doesn't matter
expect_lint("x == sort(x)", sorted_msg, linter)
expect_lint("identical(sort(x), x)", sorted_msg, linter)

# inverted version
expect_lint("sort(x) != x", unsorted_msg, linter)
expect_lint("!identical(x, sort(x))", unsorted_msg, linter)

# expression matching
expect_lint("sort(foo(x)) == foo(x)", sorted_msg, linter)
expect_lint("identical(foo(x), sort(foo(x)))", sorted_msg, linter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some more tests against false positives, like identical(sort(x), y), identical(sort(foo(x)), x), and a test that this catches expressions: identical(sort(foo(x)), foo(x))

})

test_that("lints vectorize", {
Expand Down
Loading