Remove ellipsis from scale constructors #1763

thomasp85 · 2016-09-20T11:36:19Z

thomasp85 · 2016-09-20T11:37:19Z

@Haldey - I just changed it for scale_alpha for the time being. Can you have a look and see if this approach looks good to you...

hadley · 2016-09-20T13:49:33Z

I really don't like this approach, but I don't think there's any way to do better, so I think I'd rather fix this in roxygen2, by providing someway to specify that the docs for ... should come from continuous_scale (i.e. r-lib/roxygen2#512).

That won't help with autocomplete though. Maybe @kevinushey will have some ideas.

thomasp85 · 2016-09-20T14:44:36Z

Which part is it you don't like? The forwarding of arguments or the doccumentation?

hadley · 2016-09-20T14:51:06Z

The forwarding of the arguments.

thomasp85 · 2016-09-20T14:52:31Z

I can do it manually as is done for scale_*_position - just tried to be smart

kevinushey · 2016-09-20T15:39:36Z

RStudio does not yet have the smart control-flow analysis needed to detect how ... arguments are delegated.

However, it's possible we could also look at the function documentation for available arguments, e.g. by parsing the .Rd help and reading the parameter names in there. We could then use that to provide smart autocompletion in the presence of ... arguments.

hadley · 2016-09-20T19:21:31Z

R/scale-alpha.r

@@ -19,17 +17,34 @@
 #' (p <- ggplot(mtcars, aes(mpg, cyl)) +
 #'   geom_point(aes(alpha = factor(cyl))))
 #' p + scale_alpha_discrete(range = c(0.4, 0.8))
-scale_alpha <- function(..., range = c(0.1, 1)) {
-  continuous_scale("alpha", "alpha_c", rescale_pal(range), ...)
+scale_alpha <- function(name = waiver(), range = c(0.1, 1), breaks = waiver(),


What if we wrote forward_to so that this would work:

scale_alpha <- forward_to(continuous_scale, aesthetics = "alpha", scale_name = "alpha_c", palette = rescale_pal() )

One downside is that this would require continuous_scale to be defined before scale_alpha in order to inspect the arguments.

Or what about:

forward_to(contiunous_scale, aesthetics = "alpha", scale_name = "alpha_c", palette = rescale_pal(range), range = NULL )

forward_to <- function(fun, ...) { call <- as.call(c(quote(fun), find_args(...))) eval(call) }

thomasp85 · 2016-09-21T19:33:35Z

I've made this piece of magical code that I would like you to consider (it depends on the find_args() function from #1762)

forward_to <- function(.fun, ..., .mask, .prepare) {
  args <- as.list(match.call())[-1]
  args <- args[!names(args) %in% c(".fun", ".mask", ".prepare")]
  frml <- formals(.fun)
  .prepare <- substitute(.prepare)
  .fun <- substitute(.fun)
  new_frml <- modifyList(frml[!names(frml) %in% .mask], args)
  new_fun <- function() {
    args <- find_args()
    prepared <- new.env()
    eval(.prepare, envir = prepared)
    args <- modifyList(args, as.list(prepared))
    args <- args[names(args) %in% names(frml)]
    call <- as.call(c(.fun, args))
    eval(call)
  }
  formals(new_fun) <- new_frml
  new_fun
}

with it you can do:

scale_alpha <- forward_to(continuous_scale, 
  range = c(0.1, 1), 
  .mask = c("palette", "super", "position", "rescaler", "aesthetics", "scale_name"),
  .prepare = {
    aesthetics <- "alpha"
    scale_name <- "alpha_c"
    palette <- rescale_pal(range)
  }
)

It's a bit verbose but it allow you to set new arguments with defaults, define which arguments in the destination function should be inaccessible and give freedom to manipulate and create variables prior to calling the destination function (any variable in .prepare that match the name of a formal in the destination function is passed along)

jimhester · 2016-09-21T20:41:11Z

Wouldn't the following be a simpler implementation and produce more readable function bodies, maybe I am missing a subtlety though...

forward_to <- function(f, ..., .ignore = character(), .code = NULL) {
  fmls <- modifyList(formals(f), list(...))
  fmls <- fmls[!names(fmls) %in% .ignore]
  called_fmls <- stats::setNames(lapply(names(fmls), as.symbol), names(fmls))
  call <- as.call(c(substitute(f), called_fmls))

  formals(f) <- fmls
  body(f) <- bquote({
    .(code)
    .(call)},
    as.environment(list(call = call, code = substitute(.code))))
  f
}

forward_to(continuous_scale, range = c(0.1, 1),
  .ignore = c("palette", "super", "position", "rescaler", "aesthetics", "scale_name"),
  .code = {
    aesthetics <- "alpha"
    scale_name <- "alpha_c"
    pallette <- rescale_pal(range)
  })
#> function (name = waiver(), breaks = waiver(), minor_breaks = waiver(), 
#>     labels = waiver(), limits = NULL, oob = censor, expand = waiver(), 
#>     na.value = NA_real_, trans = "identity", guide = "legend", 
#>     range = c(0.1, 1)) 
#> {
#>     {
#>         aesthetics <- "alpha"
#>         scale_name <- "alpha_c"
#>         pallette <- rescale_pal(range)
#>     }
#>     continuous_scale(name = name, breaks = breaks, minor_breaks = minor_breaks, 
#>         labels = labels, limits = limits, oob = oob, expand = expand, 
#>         na.value = na.value, trans = trans, guide = guide, range = range)
#> }
#> <environment: namespace:ggplot2>

thomasp85 · 2016-09-21T21:01:27Z

I'm so new to meta-programming and was so proud until you came along @jimhester :-)

But one thing your function is missing is to take matching variables from .code/.prepare and add them to the call to f/.fun

jimhester · 2016-09-21T21:06:41Z

Ok this should work better, looks like all the arguments are being passed properly.

forward_to <- function(f, ..., .dont_forward = character(), .dont_call = .dont_forward, .code = NULL) {
  .code <- substitute(.code)
  fmls <- formals(f)
  forwarded_fmls <- fmls[!names(fmls) %in% .dont_forward]
  called_fmls <- fmls[!names(fmls) %in% .dont_call]
  forwarded_fmls <- modifyList(forwarded_fmls, list(...))
  called_fmls <- stats::setNames(lapply(names(called_fmls), as.symbol), names(called_fmls))
  call <- as.call(c(substitute(f), called_fmls))

  formals(f) <- forwarded_fmls
  body(f) <- bquote({
    .(code)
    .(call)},
    as.environment(list(call = call, code = .code)))
  f
}

forward_to(continuous_scale, range = c(0.1, 1),
  .dont_forward = c("palette", "super", "position", "rescaler", "aesthetics", "scale_name"),
  .dont_call = c("super", "position", "rescaler"),
  .code = {
    aesthetics <- "alpha"
    scale_name <- "alpha_c"
    pallette <- rescale_pal(range)
  })
#> function (name = waiver(), breaks = waiver(), minor_breaks = waiver(), 
#>     labels = waiver(), limits = NULL, oob = censor, expand = waiver(), 
#>     na.value = NA_real_, trans = "identity", guide = "legend", 
#>     range = c(0.1, 1)) 
#> {
#>     {
#>         aesthetics <- "alpha"
#>         scale_name <- "alpha_c"
#>         pallette <- rescale_pal(range)
#>     }
#>     continuous_scale(aesthetics = aesthetics, scale_name = scale_name, 
#>         palette = palette, name = name, breaks = breaks, minor_breaks = minor_breaks, 
#>         labels = labels, limits = limits, oob = oob, expand = expand, 
#>         na.value = na.value, trans = trans, guide = guide)
#> }
#> <environment: namespace:ggplot2>

jimhester · 2016-09-21T21:20:11Z

@thomasp85 I updated the functions a bit, let me know how that looks.

thomasp85 · 2016-09-21T21:24:31Z

Except .code could give rise to variables not matching any arguments so they need to be matched to the formals of the function and potentially override those given in call

I'm on a phone so I'm reluctant to try atm but I can probably modify your version to give me what I want

jimhester · 2016-09-21T21:38:12Z

I don't think it matters what is in .code assuming you use the same names, an example of the case you are worried about would help.

thomasp85 · 2016-09-21T21:39:59Z

I'll try to break it tomorrow😉

thomasp85 · 2016-09-22T07:01:02Z

Ah, now I see what it's doing. Yeah it should work, at the expense of both defining .dont_forward and .dont_call

And I do agree that the resulting function is more comprehensible...

@hadley should I proceed down this road?

hadley · 2016-09-22T13:46:30Z

Neither of those approaches seem particularly clear to me — I'd like something that does a better job of illuminating the relationship between the two functions. Let me think about it for a bit.

thomasp85 · 2016-09-22T14:42:49Z

Wouldn't some detailed documentation and comments help?

I think you'll have a hard time finding an approach that both eleminate ellipsis, gracefully handles new arguments to continuous_scale/discrete_scale, supports new arguments and calculations prior to calling the function forwarded to, and being cognitively simple...

hadley · 2016-09-22T14:49:57Z

It's not the implementation of the forwards_to function that I'm particularly concerned about, but the code that you write with it.

thomasp85 · 2016-09-22T14:54:45Z

So it's the nature of the generated function? The last of Jims iterations almost mirrors the current source code except for the fenced block

thomasp85 · 2016-09-22T14:58:58Z

Oh you mean the verbosity of the call to forward_to?

hadley · 2016-09-22T15:09:14Z

Yeah, I don't find the call to forward_to particularly illuminating.

thomasp85 · 2016-09-22T15:15:59Z

I'll try to ponder on a better syntax

thomasp85 · 2016-09-23T08:52:48Z

What about a syntax such as this:

forward(
  args = list(range = c(0.1, 1)),
  code = {
    aesthetics <- "alpha"
    scale_name <- "alpha_c"
    pallette <- rescale_pal(range)
  },
  to = continuous_scale,
  redefine = c("palette", "aesthetics", "scale_name"),
  use.defaults = c("super", "position", "rescaler")
)

I think you can almost read that aloud and make sense of it...

forward these arguments and this code to this function, redefining these arguments and using defaults for these...

jimhester · 2016-09-23T13:10:48Z

@thomasp85 I like that syntax, I would prefer it be use_defaults, but using a period is needed to fit with the rest of ggplot2 style I suppose :(

hadley · 2016-09-27T19:03:16Z

I think this is better, but not good enough to be worth finishing off for this release.

lock · 2019-01-18T15:19:30Z

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

Try out with scale_apha

0ef095a

Fix typos

20a83e7

hadley closed this Sep 20, 2016

hadley reviewed Sep 20, 2016

View reviewed changes

thomasp85 reopened this Sep 20, 2016

thomasp85 self-assigned this Sep 22, 2016

hadley closed this Sep 27, 2016

jimhester mentioned this pull request Sep 12, 2018

Expose all installation options to all install functions r-lib/remotes#156

Merged

lock bot locked and limited conversation to collaborators Jan 18, 2019

Remove ellipsis from scale constructors #1763

Remove ellipsis from scale constructors #1763

Uh oh!

Conversation

thomasp85 commented Sep 20, 2016

Uh oh!

thomasp85 commented Sep 20, 2016

Uh oh!

hadley commented Sep 20, 2016

Uh oh!

thomasp85 commented Sep 20, 2016

Uh oh!

hadley commented Sep 20, 2016

Uh oh!

thomasp85 commented Sep 20, 2016

Uh oh!

kevinushey commented Sep 20, 2016

Uh oh!

hadley Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

hadley Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

hadley Sep 20, 2016

Choose a reason for hiding this comment

Uh oh!

hadley Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasp85 commented Sep 21, 2016

Uh oh!

jimhester commented Sep 21, 2016

Uh oh!

thomasp85 commented Sep 21, 2016

Uh oh!

jimhester commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimhester commented Sep 21, 2016

Uh oh!

thomasp85 commented Sep 21, 2016

Uh oh!

jimhester commented Sep 21, 2016

Uh oh!

thomasp85 commented Sep 21, 2016

Uh oh!

thomasp85 commented Sep 22, 2016

Uh oh!

hadley commented Sep 22, 2016

Uh oh!

thomasp85 commented Sep 22, 2016

Uh oh!

hadley commented Sep 22, 2016

Uh oh!

thomasp85 commented Sep 22, 2016

Uh oh!

thomasp85 commented Sep 22, 2016

Uh oh!

hadley commented Sep 22, 2016

Uh oh!

thomasp85 commented Sep 22, 2016

Uh oh!

thomasp85 commented Sep 23, 2016

Uh oh!

jimhester commented Sep 23, 2016

Uh oh!

hadley commented Sep 27, 2016

Uh oh!

lock bot commented Jan 18, 2019

Uh oh!

Uh oh!

hadley Sep 20, 2016 •

edited

Loading

jimhester commented Sep 21, 2016 •

edited

Loading