Update message for redundant ifelse linter #1432
Conversation
R/redundant_ifelse_linter.R
Outdated
| "as.numeric", | ||
| "as.integer" | ||
| replacement_numeric, | ||
| "as.integer(x)" |
There was a problem hiding this comment.
shouldn't ifelse(x, 0L, 1L) also recommend negation?
sorry for all the back & forth here...
There was a problem hiding this comment.
Ah, sorry, missed that case here. Yes, we should also recommend negation here.
It should be as.integer(!x), right? Since the original conditions are 0L and 1L?
x <- 8
ifelse(x > 5, 0L, 1L)
#> [1] 0
as.numeric(! (x > 5))
#> [1] 0
as.integer(! (x > 5))
#> [1] 0Created on 2022-06-28 by the reprex package (v2.0.1)
|
Getting much closer! I may have missed this in my quick glance, but I don't think we have any tests yet for the following case: Where the alternative is |
|
Great catch, @MichaelChirico! We indeed have tests for these cases. I have added them and made sure that they pass. |
| "as.numeric", | ||
| "as.integer" | ||
| ) | ||
| is_numeric_01 <- first_arg %in% c("0", "1") | second_arg %in% c("0", "1") |
There was a problem hiding this comment.
I would think a flow like
ifelse(is_numeric_01, "as.numeric", "as.integer")
is_negated <- first_arg %in% c("0", "0L")
ifelse(is_negated, "!x", "x")
then build the message with those two bits
would be simpler/easier to follow, is there a reason that won't work?
There was a problem hiding this comment.
Thanks for the suggestion!
That totally works; I was just being stingy in creating objects. But I guess this is a bit more readable.
There was a problem hiding this comment.
if we really want to skimp we don't need to allocate either is_* variable since they're not used only once. either ways basically don't though. these objects are almost always tiny (maybe a couple dozen elements in the most extreme realistic case)
R/redundant_ifelse_linter.R
Outdated
| is_numeric_01 <- first_arg %in% c("0", "1") | second_arg %in% c("0", "1") | ||
| # checking for both integer and numeric literals makes sure that mixing | ||
| # them (e.g. `ifelse(x > 5, 0, 1L)` or `ifelse(x > 5, 0L, 1)`) will provide expected lint | ||
| replacement_numeric <- ifelse(first_arg %in% c("0", "0L") & second_arg %in% c("1", "1L"), "as.numeric(!x)", "as.numeric(x)") |
There was a problem hiding this comment.
too wide, if this line is kept
NEWS.md
Outdated
| @@ -8,6 +8,8 @@ | |||
| * `seq_linter()` additionally lints on `1:n()` (from dplyr) | |||
| and `1:.N` (from data.table) (#1396, @IndrajeetPatil). | |||
|
|
|||
| * `redundant_ifelse_linter()` produces correct message (#1432, @IndrajeetPatil). | |||
There was a problem hiding this comment.
please be more descriptive in the NEWS -- assume the reader will never read this PR or the associated issues. convey enough for them to get the idea of what's changed even without consulting other docs
R/redundant_ifelse_linter.R
Outdated
| "as.integer" | ||
| ) | ||
| is_numeric_01 <- first_arg %in% c("0", "1") | second_arg %in% c("0", "1") | ||
| replacement_is_numeric <- ifelse(is_numeric_01, "as.numeric", "as.integer") |
There was a problem hiding this comment.
not: better naming. current reads like it's a boolean variable. maybe "coercion_function"?
R/redundant_ifelse_linter.R
Outdated
| is_numeric_01 <- first_arg %in% c("0", "1") | second_arg %in% c("0", "1") | ||
| replacement_is_numeric <- ifelse(is_numeric_01, "as.numeric", "as.integer") | ||
| is_negated <- first_arg %in% c("0", "0L") | ||
| replacement_is_negated <- ifelse(is_negated, "!x", "x") |
There was a problem hiding this comment.
not: ditto. maybe "replacement_argument"?
| redundant_ifelse_linter() | ||
| ) | ||
|
|
||
| # mixing int and num |
There was a problem hiding this comment.
nit: add tests for mixed 1,0 too
NEWS.md
Outdated
| @@ -8,6 +8,10 @@ | |||
| * `seq_linter()` additionally lints on `1:n()` (from dplyr) | |||
| and `1:.N` (from data.table) (#1396, @IndrajeetPatil). | |||
|
|
|||
| * `redundant_ifelse_linter()` produces correct message in the cases | |||
There was a problem hiding this comment.
much better! suggested tweak:
correctly suggests negation when the
yescondition is 0
(0 not in code so that it implies both the numeric and integer cases; and mentioning 1 is redundant since the lint only throws if 0/1 are paired)
|
Very close now thanks! small stuff left |
|
Thanks for the terrific code review! Really instructive for me to get an idea of our code style :) |
Closes #1430