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

add example of probable source of error to error message #241

Closed
ahcyip opened this issue Jun 23, 2021 · 3 comments · Fixed by #279
Closed

add example of probable source of error to error message #241

ahcyip opened this issue Jun 23, 2021 · 3 comments · Fixed by #279

Comments

@ahcyip
Copy link

ahcyip commented Jun 23, 2021

Just a very minor proposal to add to the error message at https://github.com/r-lib/rlang/blob/1a53bd085c8e0ed41dcc56cce334dde926da7067/R/dots-ellipsis.R#L169

"These dots only exist to allow future extensions and should be empty."
"Did you misspecify an argument?"

It seems a common way to reach this error is forgetting to use c() e.g. I was using any_of("a","b","c") when it should have been any_of(c("a","b","c"))

Would it be acceptable to add "Did you misspecify an argument? e.g. forget to use c()"?" to this error message? I think it could be helpful for beginners.

@lionel-
Copy link
Member

lionel- commented Jun 28, 2021

Thanks for the suggestion. I think this message would be too specific since dots_check_empty() is used in a wide array of cases.

We could look into more specific help in tidyselect though.

@lionel- lionel- transferred this issue from r-lib/rlang Jun 28, 2021
@hadley
Copy link
Member

hadley commented Aug 10, 2022

Maybe something like this?

if (!missing(...)) {
  abort(c(
    "`...` should be empty.",
    "i" = "Did you forget to use `c()`? You need `any_of(c('a', 'b'))`, not `any_of('a', 'b')`"
  ))
}

Looks like any_of() is the only place where this matters.

@lionel-
Copy link
Member

lionel- commented Aug 11, 2022

Looks good. Just needs to use "must" instead of "should".

Also "You need" sounds a bit authoritative, which could confuse users when the error is due to something else. Perhaps:

! `...` must be empty.
i Did you forget to use `c()`?
  The expected syntax is `any_of(c("a", "b"))`, not `any_of("a", "b")`.

hadley added a commit that referenced this issue Aug 11, 2022
hadley added a commit that referenced this issue Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants