New expect_setequal_linter #951
Conversation
| @@ -94,6 +94,7 @@ function calls. (#850, #851, @renkun-ken) | |||
| + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar | |||
| + `expect_named_linter()` Require usage of `expect_named(x, n)` over `expect_equal(names(x), n)` and similar | |||
| * `expect_length_linter()` Require usage of `expect_length(x, n)` over `expect_equal(length(x), n)` and similar | |||
| * `expect_setequal_lintr()` Require usage of `expect_setequal(x, y)` over `expect_equal(sort(x), sort(y))` and similar | |||
There was a problem hiding this comment.
sort(unique(x))and sort(unique(y)), no?
|
|
||
| xpath <- glue::glue("//expr[ | ||
| expr[SYMBOL_FUNCTION_CALL[text() = 'expect_identical' or text() = 'expect_equal']] | ||
| and expr[expr[SYMBOL_FUNCTION_CALL[text() = 'sort']]] |
There was a problem hiding this comment.
sort() doesn't deduplicate, so we can only ascertain the lint for sort(unique()) or unique(sort()).
There was a problem hiding this comment.
Our sense is that people are using expect_equal(sort()) in cases without dups, hence pointing out in the lint message that dup tests are different. Even with dups, expect_equal(sort(x), sort(y)) smells fishy to me -- a bit hard to imagine when such a test would not be better written in another way.
This linter only caught a handful of cases in practice as is. Maybe we should just exclude this one?
There was a problem hiding this comment.
Fine for me.
I can imagine expect_equal(sort(..), ...) to be useful when testing some join like functionality.
There was a problem hiding this comment.
Makes sense... I wonder if expect_setequal() is maybe itself undesirable -- a better test should probably specify exactly how the sets are equal, right? Maybe expect_setequal() should be replaced with expect_identical(sort(unique(x)), y) 🤔
Anyway, closing for now.
Part of #884