Skip to content
Please note that GitHub no longer supports your web browser.

We recommend upgrading to the latest Google Chrome or Firefox.

Learn more
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

Using an existing function for filtering `...` instead of writing it out each time. #231

Open
bokov opened this issue Sep 25, 2019 · 7 comments
Labels
Milestone

Comments

@bokov
Copy link
Contributor

@bokov bokov commented Sep 25, 2019

I'm watching a series of related issues and PRs here re: ....

The tidyverse/r-lib packages are using the ellipsis package for similar functionality, in case that holds any interest. Yes, it would be a dependency. But it would also eliminate the need to grow your own solution here. YMMV 🤷‍♀️

Originally posted by @jennybc in #225 (comment)

@bokov

This comment has been minimized.

Copy link
Contributor Author

@bokov bokov commented Sep 25, 2019

I'm watching a series of related issues and PRs here re: ....

The tidyverse/r-lib packages are using the ellipsis package for similar functionality, in case that holds any interest. Yes, it would be a dependency. But it would also eliminate the need to grow your own solution here. YMMV woman_shrugging

Thank you, this is a helpful package. I don't see a way to have it actually filter out unused arguments in order to prevent an error, though. This got me thinking, though-- you're right, we can't be the first people encountering the problem of ... being passed to a function which doesn't have ... as a formal argument. In fact, Hadley Wickham had this question back in 2008 and he didn't want to reinvent the wheel by implementing it himself. After some more digging I found R.utils::doCall which does exactly what we need except that it drops argument silently.

I submitted an issue to that repo and if feedback is favorable I will submit a pull request to them which implements warnings as an option.

As you kind of alluded, we don't know how @leeper will feel about adding another dependency just for this. But if so, many of the .import.rio_* methods could follow a very consistent pattern...

Instead of

.import.rio_ods <- function(file, which = 1, header = TRUE, ...) {
    requireNamespace("readODS")
    readODS::read_ods(path = file, sheet = which, col_names = header, ...)
}

we would have this...

.import.rio_ods <- function(file, which = 1, header = TRUE, ...) {
    requireNamespace("readODS")
    a <- list(...)
    a[c('path', 'sheet', 'col_names')] <- c(file, which, header)
    doCall(readODS::read_ods, a)
}

...and for many cases the only things different from one underlying import function to another are the names of values in a that get created/overwritten and the function object.

@jennybc

This comment has been minimized.

Copy link

@jennybc jennybc commented Sep 27, 2019

Thank you, this is a helpful package. I don't see a way to have it actually filter out unused arguments in order to prevent an error, though.

I think the ellipsis mentality is that we usually want to error for unused arguments. If you provided foreign arguments to a function that did not have ..., you'd get an error. So often a user would like to know that some of their arguments are being quietly sent to /dev/null.

However, sometimes this is necessary. That recently came up in devtools (https://www.tidyverse.org/articles/2019/09/devtools-2-2-1/#ellipsis), which lead to more flexibility being built into ellipsis (r-lib/ellipsis@dc23a8c).

@leeper leeper added the enhancement label Oct 3, 2019
@leeper leeper added this to the v0.6 milestone Dec 20, 2019
@leeper

This comment has been minimized.

Copy link
Owner

@leeper leeper commented Dec 20, 2019

So this is related to #132. Basically, we need:

  • a schema that provides "standard" argument names that we will use for common behavior
  • something like the crosswalk you created @bokov that maps function/package argument naming schemes to the rio standard
  • a function that takes argument lists and translates them from rio standard to package standard where appropriate, passing forward anything else, and erroring on invalid arguments

I think there's a slight annoyance that we can't use do.call() with suggested packages as the function name isn't found, ex:

> requireNamespace("data.table")
Loading required namespace: data.table
> do.call("fread", list("foo.csv"))
Error in fread("foo.csv") : could not find function "fread"
> do.call("data.table::fread", list("foo.csv"))
Error in `data.table::fread`("foo.csv") :
  could not find function "data.table::fread"
@leeper

This comment has been minimized.

Copy link
Owner

@leeper leeper commented Dec 20, 2019

Actually, it looks like that might be solvable with:

do.call(getFromNamespace("fread", "data.table"), list("foo.csv"))

https://r.789695.n4.nabble.com/do-call-method-within-namespace-td797206.html

@bokov

This comment has been minimized.

Copy link
Contributor Author

@bokov bokov commented Dec 20, 2019

So this is related to #132.

Yes, and also #245. Actually have some code in a branch for that.

@bokov

This comment has been minimized.

Copy link
Contributor Author

@bokov bokov commented Dec 20, 2019

Actually, it looks like that might be solvable with:

do.call(getFromNamespace("fread", "data.table"), list("foo.csv"))

https://r.789695.n4.nabble.com/do-call-method-within-namespace-td797206.html

Cooool! I learned something new today, thank you.

@leeper

This comment has been minimized.

Copy link
Owner

@leeper leeper commented Dec 20, 2019

So did I!

bokov added a commit to bokov/rio that referenced this issue Dec 20, 2019
@bokov bokov mentioned this issue Dec 20, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.