-
Notifications
You must be signed in to change notification settings - Fork 188
Create all_equal_linter() #2885
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2885 +/- ##
=======================================
Coverage 99.28% 99.28%
=======================================
Files 128 129 +1
Lines 7232 7270 +38
=======================================
+ Hits 7180 7218 +38
Misses 52 52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a7111d8
to
a5e900b
Compare
#' preceded by the negation operator `!`, are thus likely to generate unexpected | ||
#' errors if the compared objects have differences. | ||
#' An alternative is to use `identical()` to compare vector of strings or when | ||
#' exact equality is expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW stopifnot()
has some built-in logic for handling all.equal()
checks, c.f.
stopifnot(all.equal(1, 1.001))
# Error: 1 and 1.001 are not equal:
# Mean relative difference: 0.001
I might actually lint this code while we're at it:
stopifnot(isTRUE(all.equal(x, y)))
Since it gives a worse error:
stopifnot(isTRUE(all.equal(1, 1.001)))
# Error: isTRUE(all.equal(1, 1.001)) is not TRUE
Some hits:
We can do that as a follow-up if you'd like; in this PR at least I'd want to have some mention of stopifnot()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well test isFALSE(all.equal())
here too, it gets some usage:
https://github.com/search?q=lang%3AR+%2FisFALSE%5C%28all%5C.equal%5C%28%2F&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, isFALSE(all.equal())
is definitely wrong since it'll always return FALSE
. I wonder if that'd fit better under a istrue_linter()
that check cases where authors wanted !isTRUE()
rather than isFALSE()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly for stopifnot()
, great catch but I wonder if it should live here or in stopifnot_all_linter()
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFALSE(all.equal(...))
is pretty rare, I guess it doesn't warrant its own linter. Let's put it here.
I also think the stopifnot()
bit belongs here, since it's about incorrect usage of all.equal()
, whereas stopifnot_all_linter()
is about incorrect usage of stopifnot()
. WDYT @AshesITR / @IndrajeetPatil ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I view stopifnot_all_linter()
as "stopifnot(check)
has actually more bells and whistles than if (check) stop()
". You can pass it all.equal()
without isTRUE()
the same way that you can pass it a logical vector without all()
,
So it seems like a good fit there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thing to consider is that conjunct_test_linter()
already has some logic related to stopifnot()
:
lintr/R/conjunct_test_linter.R
Line 110 in dc8f149
stopifnot_calls <- source_expression$xml_find_function_calls("stopifnot") |
If we put the new all.equal()
lint inside stopifnot_all_linter()
, I'd want to rename it to just stopifnot_linter()
, which would also create a tension with conjunct_test_linter()
about where the check on stopifnot(A && B)
should live. Fun :)
Not something that needs to be addressed in this PR: #2905
@@ -51,6 +51,7 @@ | |||
|
|||
### New linters | |||
|
|||
* `all_equal_linter()` warns about incorrect use of `all.equal()` in `if` clauses or preceded by `!` (#2885, @Bisaloo). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revisit the wording here
Linter(linter_level = "expression", function(source_expression) { | ||
all_equal_calls <- source_expression$xml_find_function_calls("all.equal") | ||
|
||
dangerous_unwrapped_all_equal <- xml_find_all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic is slightly off -- we get lints here in r-devel:
https://github.com/r-devel/r-svn/blob/6d2efbc65c5ae1c15ce4f1e25e2fcf5ddc3752d2/src/library/base/R/all.equal.R#L225-L226
https://github.com/r-devel/r-svn/blob/6d2efbc65c5ae1c15ce4f1e25e2fcf5ddc3752d2/src/library/base/R/eigen.R#L38-L41
I guess we're not being careful about which expr
under IF
is being linted:
if (A) all.equal(x, y)
Found some true positives :) |
Fix #2610