Skip to content
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

Recommend using all_of() for looking up variables #127

Merged
merged 2 commits into from
Oct 18, 2019
Merged
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
22 changes: 22 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,28 @@

# tidyselect (development)

* Selection helpers like `all_of()` and `starts_with()` are now
available in all selection contexts, even when they haven't been
attached to the search path. The most visible consequence of this
change is that it is now easier to use selection functions without
attaching the host package:

```r
# Before
dplyr::select(mtcars, dplyr::starts_with("c"))

# After
dplyr::select(mtcars, starts_with("c"))
```

It is still recommended to export the helpers from your package so
that users can easily look up the documentation with `?`.

* Selecting non-column variables with bare names now triggers an
informative message suggesting to use `all_of()` instead. Referring
to contextual objects with a bare name is brittle because it might
be masked by a data frame column. Using `all_of()` is safe (#76).

* Using arithmetic operators in selection context now fails more
informatively (#84).

Expand Down
7 changes: 6 additions & 1 deletion R/vars-select.R
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ vars_select_eval <- function(vars, quos) {
# Add `.data` pronoun in the context mask even though it doesn't
# contain data. This way the pronoun can be used in any parts of the
# expression.
context_mask <- new_data_mask(env())
context_mask <- new_data_mask(env(!!!vars_select_helpers))
context_mask$.data <- data_mask$.data
}

Expand Down Expand Up @@ -628,6 +628,11 @@ eval_sym <- function(name, data_mask, context_mask, colon = FALSE) {
)

if (!is_missing(value)) {
inform(glue_c(
"Note: Selecting non-column variables is brittle.",
i = "If the data contains `{name}` it will be selected instead.",
i = "Use `all_of({name})` to silence this message."
))
return(value)
}

Expand Down
8 changes: 8 additions & 0 deletions tests/testthat/outputs/vars-select-context-lookup.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
> vars_select(letters, vars)
Message: Note: Selecting non-column variables is brittle.
i If the data contains `vars` it will be selected instead.
i Use `all_of(vars)` to silence this message.

a b
"a" "b"

19 changes: 19 additions & 0 deletions tests/testthat/test-vars-select-eval.R
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,22 @@ test_that("can't use arithmetic operators in data context", {
test_that("can use arithmetic operators in non-data context", {
expect_identical(vars_select(letters, identity(2 * 2 + 2 ^ 2 / 2)), c(f = "f"))
})

test_that("symbol lookup outside data informs caller about better practice", {
vars <- c("a", "b")
expect_message(
vars_select(letters, vars),
"Use `all_of(vars)` to silence",
fixed = TRUE
)
verify_output(test_path("outputs", "vars-select-context-lookup.txt"), {
vars_select(letters, vars)
})
})

test_that("selection helpers are in the context mask", {
out <- local(envir = baseenv(), {
tidyselect::vars_select(letters, all_of("a"))
})
expect_identical(out, c(a = "a"))
})