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

Checking ... #3196

Closed
ColinFay opened this issue Mar 20, 2019 · 12 comments · Fixed by #6031
Closed

Checking ... #3196

ColinFay opened this issue Mar 20, 2019 · 12 comments · Fixed by #6031

Comments

@ColinFay
Copy link

Following this twitter thread.

labs() uses ..., but the name of the arguments are not checked, so this can lead to errors when passing tile = instead of title, or any other typo.

It would be nice for the user to have at least a warning (or an error) here:

library(ggplot2)
ggplot(iris, aes(Sepal.Length, Sepal.Width)) + 
  geom_point() + 
  labs(tile = "Title has a typo")
@hadley
Copy link
Member

hadley commented Mar 21, 2019

And in all other functions that use ...

@yutannihilation
Copy link
Member

Does this require us to replace all list() with rlang::list2()? Otherwise, list() actually uses ....

f1 <- function(..., title = "foo") {
  ellipsis::check_dots_used()
  
  list(..., title = title)
}

f2 <- function(..., title = "foo") {
  ellipsis::check_dots_used()
  
  rlang::list2(..., title = title)
}

f1(tile = "Title has a typo")
#> $tile
#> [1] "Title has a typo"
#> 
#> $title
#> [1] "foo"
f2(tile = "Title has a typo")
#> Warning: Some components of ... were not used: tile
#> $tile
#> [1] "Title has a typo"
#> 
#> $title
#> [1] "foo"

Created on 2019-03-24 by the reprex package (v0.2.1)

@yutannihilation
Copy link
Member

Hmm, I come to think we can do nothing for labs(); there's a chance tile is a valid aesthetic, and labs() takes arbitrary aesthetics, which means we cannot verify the arguments without annoying false positive warnings. For example:

library(ggplot2)

labs <- function (..., title = waiver(), subtitle = waiver(), caption = waiver(), 
          tag = waiver()) {
  ellipsis::check_dots_used()
  
  args <- rlang::list2(..., title = title, subtitle = subtitle, caption = caption, tag = tag)
  
  is_waive <- vapply(args, ggplot2:::is.waive, logical(1))
  args <- args[!is_waive]
  args <- args[!duplicated(names(args))]
  args <- ggplot2:::rename_aes(args)
  
  structure(args, class = "labels")
}

labs(tile = "Title has a typo")
#> Warning: Some components of ... were not used: tile
#> $tile
#> [1] "Title has a typo"
#> 
#> attr(,"class")
#> [1] "labels"

labs(colour = "colour is a valid aesthetic")
#> Warning: Some components of ... were not used: colour
#> $colour
#> [1] "colour is a valid aesthetic"
#> 
#> attr(,"class")
#> [1] "labels"

Created on 2019-03-24 by the reprex package (v0.2.1)

@thomasp85
Copy link
Member

We could do something at build time if we really wants to because all named aesthetics is known there. Not sure it is worth it, though

@yutannihilation
Copy link
Member

Thanks. For this particular issue, I agree it's build time if we do some verification.

@yutannihilation yutannihilation changed the title Checking ... in labs() Checking ... Mar 28, 2019
@yutannihilation
Copy link
Member

To clarify, this issue is a general discussion about checking .... It seems we need different approaches for different places. For example:

@atusy
Copy link
Contributor

atusy commented Mar 29, 2019

At least for scale_x|y_*, modifying formals is a user friendly way.
However, I'm not sure for others.

For example, see scale_alpha_continuous2 implemented by modifying formals of continuous_scale, which works in the same way as original scale_alpha_continuous.
This implementation enables checking arguments on run and suggesting arguments on IDE.
However, first 3 arguments of scale_alpha_continuous2 are aesthetics, scale_name, and palette, which are not intended to be user-specified in most cases.

Thus, I guess reordering of formals may also help users so that aesthetics, scale_name, and palette go to the tail of formals.

In this way, 1st to the 5th arguments of scale_alpha_continuous2 becomes "name", "breaks", "minor_breaks", "labels", and "limits", which are consistent to the order of formals of scale_x|y_continuous.

library(ggplot2)
library(scales)
scale_alpha_continuous2 <- continuous_scale
formals(scale_alpha_continuous2)[c(
  "aesthetics", "scale_name", "palette", "range"
  )] <-
  alist("alpha", "alpha_c", rescale_pal(range), c(0.1, 1))
qplot(x=1:10, y=1:10, alpha = 1:10) +
  scale_alpha_continuous2()

Created on 2019-03-29 by the reprex package (v0.2.1)

@hadley
Copy link
Member

hadley commented Mar 29, 2019

I do not think modifying formals is the right way to approach this problem.

@yutannihilation
Copy link
Member

I too feel modifying formals might not be the direct answer here, but I think the point is that in some cases we don't really need ... just to create the variations of functions with different defaults. I don't think we have any nice way to do this (especially, in terms of roxygen's support), but, to me, it seems interesting to generate functions programmatically.

@hadley
Copy link
Member

hadley commented Jun 17, 2019

A good next step on this issue would be to systematically survey all use of ... and figure out what category they fall into

@paleolimbot
Copy link
Member

It's a bit too long for an issue comment, but I put a summary of all 217 uses (excluding s3 generics and methods) here: https://gist.github.com/paleolimbot/708effa39e67bb53f54f30499562587b

@teunbrand
Copy link
Collaborator

I reviewed all exported functions using ... and the following appear problematic:

  • fortify()
  • get_alt_text()
  • Some guide constructors, via deprecated_guide_args()
  • guides()
  • labeller()
  • labs()

Here is some code illustrating the issues:

library(ggplot2)

# fortify doesn't warn
p <- ggplot(mpg, aes(displ, hwy, colour = drv), foo = "bar") +
  geom_point() +
  guides(
    # deprecated_guide_args() doesn't warn
    colour = guide_legend(non = "sense"),
    # non existing aesthetics are silently accepted
    foobar = "qux" 
  ) +
  facet_wrap(
    ~ year,
    # labeller args are ignored
    labeller = labeller(year = label_both, nonsense = label_value)
  ) + 
  labs(
    alt = "my alt text",
    # unknown labels are ignored
    i_dont = "exist"
  )

# Extra arguments are ignored
get_alt_text(p, hamspam = "green eggs")
#> [1] "my alt text"

Created on 2024-08-06 with reprex v2.1.1

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.

7 participants