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

one_of() with arguments whose names are used in the columns of results an error #2184

Closed
ngr-t opened this issue Oct 19, 2016 · 9 comments
Closed

Comments

@ngr-t
Copy link

@ngr-t ngr-t commented Oct 19, 2016

When I use a variable name used as a column name of .data, select() and one_of() raises an error.
Below is an example.

> library(dplyr)
> col <- Species <- "Sepal.Width"
> # variable name "Species" (of course it's one of a column name of iris data) is invalid
> iris %>% select(one_of(Species))
Error: `c(...)` must be a character vector
> iris %>% select(one_of(col)) %>% head
  Sepal.Width
1         3.5
2         3.0
3         3.2
4         3.1
5         3.6
6         3.9
@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 7, 2016

Thanks, confirmed. Would you like to contribute a testthat test?

@ngr-t ngr-t added the bug label Nov 7, 2016
@ngr-t
Copy link
Author

@ngr-t ngr-t commented Nov 10, 2016

I'd gladly to do so.
I feel that the test should be included in tests/testthat/test-select-helpers.R, is it OK?

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 10, 2016

I was confused. I think you need to do one_of("Species").

@ngr-t
Copy link
Author

@ngr-t ngr-t commented Nov 10, 2016

Not. Sorry for poor explanation.
In the example above, the expected return value is a data.frame which has one column of "Sepal.Width" because the content of Species is "Sepal.Width".
My point is that accessing with a variable whose name is included by the column names causes an error.
It might be problematic when we write some function which takes data.frames as its parameters and don't know the column names of them.

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 11, 2016

I see:

iris %>% select(one_of(Species))

doesn't work (and isn't supposed to) if there is no Species variable. But as soon as you assign a value to Species, it's supposed to work, but doesn't:

Species <- "Petal.Width"
iris %>% select(one_of(Species))

It works if you use a variable that doesn't match the column name:

col <- "Petal.Width"
iris %>% select(one_of(col))

I think test-select-helpers.R is a good place.

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 11, 2016

Are other select helpers affected?

ngr-t added a commit to ngr-t/dplyr that referenced this issue Nov 24, 2016
ngr-t added a commit to ngr-t/dplyr that referenced this issue Dec 13, 2016
krlmlr added a commit to krlmlr/dplyr that referenced this issue Feb 9, 2017
krlmlr added a commit that referenced this issue Feb 10, 2017
* Update test-select-helpers.R

added test for issue #2184

* Applied required changes.

* added tests for helper functions other than `one_of()`

* fix test

* tweak

* fix issue number
krlmlr added a commit to krlmlr/dplyr that referenced this issue Feb 10, 2017
@hadley hadley added the nse label Feb 20, 2017
@hadley
Copy link
Member

@hadley hadley commented Feb 22, 2017

It's not actually clear that this is a bug. Generally, you should avoid naming the variables you use inside one_of() the same as variables in your data frame. But @lionel- will take a deeper look.

@lionel-
Copy link
Member

@lionel- lionel- commented Feb 22, 2017

I fixed it by evaluating all calls supplied to select_vars() in the calling environment. Except for calls known to work with columns like :.

@lionel-
Copy link
Member

@lionel- lionel- commented Mar 15, 2017

Now fixed on master

@lionel- lionel- closed this Mar 15, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jun 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants