Fix if_any() and if_all() behavior with zero-column selections
#7072
Conversation
Fixes tidyverse#7059 This commit addresses the unexpected behavior of `if_any()` and `if_all()` when no columns are selected. Specifically, `if_any()` now returns `FALSE`, and `if_all()` returns `TRUE` in cases where no columns match the selection criteria. Additionally, tests have been added to ensure the correct behavior of these functions in both `filter()` and `mutate()` contexts, including the scenarios highlighted by the `reprex` example. This ensures that empty selections behave consistently with the logical expectations.
tests/testthat/test-across.R
Outdated
|
|
||
| expect_equal( | ||
| filter(tbl, if_any(c(), ~ is.na(.x))), | ||
| filter(tbl, FALSE) |
There was a problem hiding this comment.
Expected values seem slightly better without too much logic, not sure if it's easier to read though:
| filter(tbl, FALSE) | |
| tbl[0, ] |
There was a problem hiding this comment.
Thanks for the feedback!
tests/testthat/test-across.R
Outdated
|
|
||
| expect_equal( | ||
| filter(tbl, if_all(c(), ~ is.na(.x))), | ||
| filter(tbl, TRUE) |
There was a problem hiding this comment.
| filter(tbl, TRUE) | |
| tbl |
R/across.R
Outdated
| #' **Important Behavior:** When there are no selected columns: | ||
| #' | ||
| #' - `if_any()` will return `FALSE`, consistent with the behavior of | ||
| #' `any()` when called without inputs. | ||
| #' - `if_all()` will return `TRUE`, consistent with the behavior of | ||
| #' `all()` when called without inputs. |
There was a problem hiding this comment.
I would create a new @details section and move this down there, without the Important Behavior header
R/across.R
Outdated
| #' `any()` when called without inputs. | ||
| #' - `if_all()` will return `TRUE`, consistent with the behavior of | ||
| #' `all()` when called without inputs. | ||
| #' | ||
| #' If you just need to select columns without applying a transformation to each | ||
| #' of them, then you probably want to use [pick()] instead. |
There was a problem hiding this comment.
@jrwinget since you are touching documentation, can you please run devtools::document() as well and commit the results?
R/across.R
Outdated
| if (!length(quos)) { | ||
| return(list(quo(TRUE))) | ||
| return(list(quo(empty))) |
There was a problem hiding this comment.
| return(list(quo(empty))) | |
| return(list(quo(!!empty))) |
@krlmlr is right but i still think that having the empty variable is a little clearer, so here is the change you can make
There was a problem hiding this comment.
That makes sense. Thanks!
tests/testthat/test-across.R
Outdated
| @@ -888,6 888,32 @@ test_that("if_any() and if_all() expansions deal with no inputs or single inputs | |||
| ) | |||
| }) | |||
|
|
|||
| test_that("if_any() on zero-column selection behaves as expected in filter", { | |||
There was a problem hiding this comment.
| test_that("if_any() on zero-column selection behaves as expected in filter", { | |
| test_that("if_any() on zero-column selection behaves like any() (#7059)", { |
We often like to link to the issue when we add a test specifically for it
There was a problem hiding this comment.
No problem - will update that!
tests/testthat/test-across.R
Outdated
| ) | ||
| }) | ||
|
|
||
| test_that("if_all() on zero-column selection behaves as expected in filter", { |
There was a problem hiding this comment.
| test_that("if_all() on zero-column selection behaves as expected in filter", { | |
| test_that("if_all() on zero-column selection behaves like all() (#7059)", { |
There was a problem hiding this comment.
@DavisVaughan I've made the requested changes and pushed the updates. Please take a look when you have a moment. Thanks!!
- Moved specific details into the `@details` section of the documentation. - Ran `devtools::document()` to update Rd files. - Added a bullet in `NEWS.md` to highlight the consistent behavior of `if_any()` and `if_all()` with `any()` and `all()` when called with empty inputs.
|
Thanks @jrwinget! |
|
Of course - this was fun! Thanks again for your feedback!! |
Fixes #7059
This commit addresses the unexpected behavior of
if_any()andif_all()when no columns are selected. Specifically,if_any()now returnsFALSE, andif_all()returnsTRUEin cases where no columns match the selection criteria.Additionally, tests have been added to ensure the correct behavior of these functions in both
filter()andmutate()contexts, including the scenarios highlighted by thereprexexample. This ensures that empty selections behave consistently with the logical expectations.