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

Adds support of rlang::list2 dynamic dots #4764

Merged
merged 4 commits into from Apr 19, 2022
Merged

Conversation

mone27
Copy link
Contributor

@mone27 mone27 commented Mar 18, 2022

Migrates the dynamic dots management (...) from base::list to rlang::list2, see #4752

The functions modified are the one of the Public API that accepts ... as arguments.

In total 89 functions have been update, they include:

  • geom_*
  • stat_*
  • lims, expand_limits
  • aes_, aes_string
  • coord_map
  • annatation, annotation_map, annotation_logticks

Here there is the script that I have used to find the functions that use ... with list https://gist.github.com/mone27/285f022b9efbb4bc244acb6622f3411e

The PR should be complete, possible additional steps are:

@hadley
Copy link
Member

hadley commented Mar 23, 2022

This seems like a reasonable idea to me. It's probably fine to not worry about the documentation (although can you please add a news bullet) and even the tests are iffy, because what you really want to test is that every user facing ... uses dynamic-dots, and I don't see an cheap/easy way to test that.

@yutannihilation
Copy link
Member

If the intention is to prohibit the use of list completely like we do on data.frame, we can add a test here:

https://github.com/tidyverse/ggplot2/blob/main/tests/testthat/test-prohibited-functions.R

(But, as this test is a bit tricky, we can leave it for later)

@mone27
Copy link
Contributor Author

mone27 commented Mar 24, 2022

ok! I have updated NEWS.md, so I think that the PR should be complete now.

@hadley hadley merged commit 379597d into tidyverse:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants