Remove ellipsis from theme by thomasp85 · Pull Request #1762 · tidyverse/ggplot2

thomasp85 · 2016-09-20T10:15:20Z

thomasp85 · 2016-09-20T10:16:57Z

R/theme.r

+  elements <- as.list(environment())
+  elements$complete <- NULL
+  elements$validate <- NULL
+  elements <- elements[lapply(elements, class) != "name"]


I would love some feedback on whether this is the best way to capture all non-missing arguments as evaluated values...?

hadley · 2016-09-20T13:37:41Z

R/theme.r

+#' @param rect all rectangular elements (\code{element_rect})
+#' @param text all text elements (\code{element_text})
+#' @param title all title elements: plot, axes, legends (\code{element_text};
+#' inherits from \code{text})


Can you please indent the second and subsequent lines?

hadley · 2016-09-20T13:39:41Z

R/theme.r

+  elements <- c(as.list(environment()), list(...))
+  elements$complete <- NULL
+  elements$validate <- NULL
+  elements <- elements[lapply(elements, class) != "name"]


Why is this necessary? Regardless, it would be cleaner to use vapply() + is.name()

If we don't remove name elements the theme validation will complain

But why? Where do those name elements come from?

these are missing arguments - they exist in the environment and is added to the list as name objects

hadley · 2016-09-20T13:41:59Z

R/theme.r

+                  plot.caption, plot.margin, strip.background, strip.text,
+                  strip.text.x, strip.text.y, strip.switch.pad.grid,
+                  strip.switch.pad.wrap, ..., complete = FALSE, validate = TRUE) {
+  elements <- c(as.list(environment()), list(...))


Hmmmm, I don't like this but I don't see a better way except creating the massive named list by hand (which seems likely to be error prone).

I would really prefer if this was extracted into a separate function and gained a couple of safe guards:

find_args <- function(...) { env <- parent.frame() args <- names(formals(sys.function(sys.parent(1)))) vals <- c(as.list(env)[args], list(...)) vals <- vals[!vapply(vals, identical, quote(expr = ), FUN.VALUE = logical(1))] vals } f <- function(a = 1, b = 2, c = 3, d, e, abcdefasdfgasdfasdfasd) { find_args() } f(10, abc = 10)

This is slightly more elegant:

find_args <- function(...) { env <- parent.frame() args <- names(formals(sys.function(sys.parent(1)))) vals <- mget(args, envir = env) vals <- vals[!vapply(vals, identical, quote(expr = ), FUN.VALUE = logical(1))] c(vals, list(...)) }

is_missing_arg <- function(x) identical(x, quote(expr = )))

And even better:

find_args <- function(...) { env <- parent.frame() args <- names(formals(sys.function(sys.parent(1)))) vals <- mget(args, envir = env) vals <- vals[!vapply(vals, is_missing_arg, logical(1))] modifyList(vals, list(...)) }

hadley · 2016-09-20T15:47:11Z

@wch I have vague memory you thought about this problem (capturing all arguments) a bit

hadley · 2016-09-20T19:10:33Z

R/theme.r

+  elements <- c(as.list(environment()), list(...))
+  elements$complete <- NULL
+  elements$validate <- NULL
+  elements <- elements[!vapply(elements, is.name, logical(1))]


The absolutely correct check is:

elements <- elements[!vapply(elements, identical, quote(expr = ), FUN.VALUE = logical(1))]

# Conflicts: # R/theme.r # man/theme.Rd # tests/testthat/test-theme.r

hadley · 2016-09-21T12:36:11Z

R/utilities.r

+# named .ignore
+# @param ... passed on in case enclosing function uses ellipsis in argument list
+# @param .ignore remove these arguments from the list
+find_args <- function(..., .ignore = NULL) {


Why not drop .ignore and just use (e.g.) complete = NULL, validate = NULL)?

Because validate_theme will throw an error if they are included in the list

find_args(..., complete = NULL, validate = NULL)

Then modifyList will remove them

Ah - didn't think of that

hadley · 2016-09-21T12:36:41Z

NEWS.md

 * Themes are more homogeneous visually, and match `theme_grey` better.
  (@jiho, #1679)

+* The `theme()` constructor now has named arguments rather than ellipsis.


"This should make autocomplete substantially more useful"

codecov-io · 2016-09-22T07:14:07Z

Current coverage is 66.18% (diff: 100%)

Merging #1762 into master will increase coverage by 0.04%

@@             master      #1762   diff @@
==========================================
  Files           157        157          
  Lines          5112       5119     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3381       3388     +7   
  Misses         1731       1731          
  Partials          0          0

Powered by Codecov. Last update 70ae16c...ecf92c6

hadley · 2016-09-22T13:07:44Z

R/utilities.r

+# Get all arguments in a function as a list. Will fail if an ellipsis argument
+# named .ignore
+# @param ... passed on in case enclosing function uses ellipsis in argument list
+# @param .ignore remove these arguments from the list


This needs to be removed now, other LGTM

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

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/

thomasp85 added 2 commits September 20, 2016 12:13

Modify theme to have named arguments

6a97bed

Merge branch 'hadley/master' into theme-no-ellipsis

6dfa732

thomasp85 commented Sep 20, 2016

View reviewed changes

thomasp85 added 2 commits September 20, 2016 13:11

Include ellipsis

7468af6

Fix unit tests

cbfdb0e

hadley requested changes Sep 20, 2016

View reviewed changes

Indent multiline params

d946c6f

Change to use vapply instead of lapply

89fb13d

thomasp85 self-assigned this Sep 20, 2016

hadley reviewed Sep 20, 2016

View reviewed changes

thomasp85 added 3 commits September 21, 2016 10:02

Implement find_args

90d0bbb

Merge branch 'hadley/master' into theme-no-ellipsis

67fe8de

# Conflicts: # R/theme.r # man/theme.Rd # tests/testthat/test-theme.r

Unit tests for find_args

ecf92c6

hadley reviewed Sep 21, 2016

View reviewed changes

thomasp85 mentioned this pull request Sep 21, 2016

Remove ellipsis from scale constructors #1763

Closed

Remove.ignore argument from find_args

d8e30a0

Spell out advantages in NEWS :-)

3e2225d

hadley reviewed Sep 22, 2016

View reviewed changes

Remove ref to non-existent function arg

73a4201

thomasp85 merged commit e1c800d into tidyverse:master Sep 23, 2016

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

Conversation

thomasp85 commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

hadley commented Sep 20, 2016

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!

hadley Sep 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 22, 2016

Current coverage is 66.18% (diff: 100%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hadley Sep 20, 2016 •

edited

Loading

hadley Sep 20, 2016 •

edited

Loading

hadley Sep 20, 2016 •

edited

Loading

hadley Sep 21, 2016 •

edited

Loading