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

Alternative way of passing arguments to methods #48

Closed
moodymudskipper opened this issue Oct 3, 2022 · 7 comments · Fixed by #56
Closed

Alternative way of passing arguments to methods #48

moodymudskipper opened this issue Oct 3, 2022 · 7 comments · Fixed by #56
Milestone

Comments

@moodymudskipper
Copy link
Collaborator

@krlmlr made an interesting suggestion

Current implementation and ideas in #34 pose a few challenges :

  • Documentation is awkward
  • We sometimes have a lot of arguments to pass to the next ellipsis, it looks bad, lengthen the code and is error prone
  • Argument names are prefixed, it's not super pretty
  • no autocomplete
  • no easy way to store a constructive "template"

The suggestion, is to use additional functions that return "option" objects. These objects are fed unnamed to the ... and are recognised by the relevant method (we need a helper for this too).

This would additionally provide a way to isolate the argument checking/processing from the logic.

The downside is :

  • nested function calls (means if package not attached needs constructive:: several times)
  • we need to find cute names for these functions
  • We'd need extra work for NSE (but we don't use it/need it here)

The upside seems bigger though.

@krlmlr
Copy link
Contributor

krlmlr commented Oct 3, 2022

Thanks. I don't understand "extra work for NSE".

@moodymudskipper
Copy link
Collaborator Author

moodymudskipper commented Oct 3, 2022

Here is a POC:

# krlmlr said this needed the ellipsis but I'm not sure why
# this is exported, documented, and referenced in `?construct`
opts_function <- function(trim = NULL, as.function = FALSE, zap_srcref = FALSE, construct_env = FALSE) {
  # insert check and processing of args here
  structure(
    class = c("constructive_options", "constructive_options_function"),
    list(
      trim = trim, 
      as.function = as.function, 
      zap_srcref = zap_srcref, 
      construct_env = construct_env
    )
  )
}

fetch_opts <- function(suffix, ...) {
  class <- paste0("constructive_options_", suffix)
  Filter(function(x) inherits(x, class) ,list(...))[[1]]
}

construct_idiomatic.function <- function(x, ...) {
  args <- fetch_opts("function", ...)
  args
  # actual logic, we can pass the dots without further processing
}

construct <- function(x, ...) {
  if (is.function(x)) construct_idiomatic.function(x, ...)
}

construct(ave, opts_function(zap_srcref = TRUE))
#> $trim
#> NULL
#> 
#> $as.function
#> [1] FALSE
#> 
#> $zap_srcref
#> [1] TRUE
#> 
#> $construct_env
#> [1] FALSE
#> 
#> attr(,"class")
#> [1] "constructive_options"          "constructive_options_function"

How to use a template ?

  • we can build a list of opts_*()calls and feed them to construct() as a general template using !!!
  • or we can use a construct_template() helper so users don't need to speak rlang.

How to modify options and propagate a new value ?

We can copy args and feed it right before the forwarded dots and the updated version will be considered.

@moodymudskipper
Copy link
Collaborator Author

I don't understand "extra work for NSE".

@krlmlr It doesn't matter for this package but I was thinking that in general to use the approach used above in my POC, having NSE arguments to opts_* functions would require a bit of rlang enquo magic. We could still make it work nonetheless.

@krlmlr
Copy link
Contributor

krlmlr commented Oct 4, 2022

The ellipsis in the front enforces the use of named arguments, and provides an opportunity for better error messages and for renaming options.

We could also do a separate help page for each option constructor.

@moodymudskipper
Copy link
Collaborator Author

moodymudskipper commented Oct 4, 2022

The ellipsis in the front enforces the use of named arguments, and provides an opportunity for better error messages and for renaming options.

Right I forgot this detail thanks

We could also do a separate help page for each option constructor.

Yes I had this in mind, I meant exported, documented on its own, and linked in ?construct

@moodymudskipper
Copy link
Collaborator Author

moodymudskipper commented Oct 4, 2022

fetch_opts() above is incorrect because we need defaults when no arg has been passed, something like this should work:

fetch_opts <- function(suffix, ...) {
  class <- paste0("constructive_options_", suffix)
  Filter(function(x) inherits(x, class) ,list(...))[[1]] %||% match.fun(paste0("opts_", suffix))()
}

@moodymudskipper
Copy link
Collaborator Author

moodymudskipper commented Oct 7, 2022

Regarding the template, I now believe it should be a separate argument template = getOption("constructive_opts_template").

This simplify the handling of the ..., always unnamed, always inherits from constructive_options, never provide the same option helper twice.

This way the user can personalise once for all the way they want construct() to work, this function should be quick and easy to call and it should be easy to save preferences.

template is a simple list.

We should be able to integrate general options and waldo options to the template too, to maybe these should be wrapped into opts_* functions, maybe with a different name structure to avoid conflicts.
Note that since we don't fail anymore by default with check=NULL, the waldo options are not as useful anymore so it's ok to make them a bit less easy to toggle. We might leave data, check and one_liner out of the template, and have a opts_general() for the rest.

arguments passed to ... will override the template.

@moodymudskipper moodymudskipper added this to the 0.1 milestone Oct 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants