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

No sanity check for 'by' argument in _join functions #2091

Closed
russellpierce opened this issue Aug 25, 2016 · 3 comments
Closed

No sanity check for 'by' argument in _join functions #2091

russellpierce opened this issue Aug 25, 2016 · 3 comments

Comments

@russellpierce
Copy link
Contributor

@russellpierce russellpierce commented Aug 25, 2016

Given that many of the dplyr functions operate according to NSE it might be nice if the by argument of the _join functions did some sanity checking of type to be character or NULL. This error message is a bit unclear (to me at least):

> iris$id <- 1:nrow(iris)
> left_join(iris[,c(6, 1:3)], iris[,c(6,4:5)], by = id)
Error in duplicated.default(by) : duplicated() applies only to vectors
@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 7, 2016

The error message is different in the current dev version:

Error in UseMethod("common_by", by) : 
  no applicable method for 'common_by' applied to an object of class "function"

It changes to object ... not found if there is no function of that name. It looks difficult to fix this if we don't want to break working code (that might use a real variable to specify "by").

@hadley: Do we want to introduce a new argument by_cols and deprecate by, perhaps in the context of #557?

@hadley
Copy link
Member

@hadley hadley commented Nov 7, 2016

For now, we could just add a common_by.default with nicer text.

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 7, 2016

#' @export
common_by.default <- function(by, x, y) {
  stop("by must be a (named) character vector, a list, or NULL for natural joins (not recommended in production code)",
       call. = FALSE)
}

@krlmlr krlmlr closed this Nov 7, 2016
@krlmlr krlmlr reopened this Nov 7, 2016
@hadley hadley closed this in 2fa6e72 Feb 20, 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
3 participants