-
Notifications
You must be signed in to change notification settings - Fork 146
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
Add tidyeval to hcaes
and mutate_mapping
#423
Conversation
same strategy as Hadley's here: tidyverse/ggplot2@d697622
Hey @cnolanminich , This THANK YOU is not enough to express all my gratitude. I will test to merge this PR. Hope @PaulC91 @peeyooshc @tungmilan can test after the merge. Thanks again and give me your name/email if you want to be added to the DESCRIPTION file as contributor. |
@jbkunst thank you for this excellent package! I added my name to the |
Okay I've finished testing with both I think it would be a good idea to include at least one working example with this PR either in the NEWS.md or somewhere else Here is an example taken from the issue that I opened few weeks ago
|
Testing old examples:
Session Info:
|
R/api-hc-hc_add_series.R
Outdated
|
||
if(drop) | ||
data <- select_(data, .dots = names(mapping)) | ||
newv <- quos(names(mapping)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reuse newv
which is already defined in line # 409?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tungmilan 👍 good call --
I realized while doing this that it would be better to use rlang::syms()
vs rlang::quos()
, as in the following in muate_mapping
. If using rlang::quos()
and re-using newv
I get the following error: Error:
newvmust resolve to integer column positions, not a list
. Anyway, I think syms()
makes more sense anyway.
If you agree, I can push the version using syms()
and we can test again :)
mutate_mapping <- function(data, mapping, drop = FALSE) {
stopifnot(is.data.frame(data), inherits(mapping, "hcaes"), inherits(drop, "logical"))
# https://stackoverflow.com/questions/45359917/dplyr-0-7-equivalent-for-deprecated-mutate
# https://www.johnmackintosh.com/2018-02-19-theory-free-tidyeval/
tran <- as.character(mapping)
newv <- names(mapping)
list_names <- setNames(tran, newv) %>% lapply(rlang::parse_quosure)
data <- dplyr::mutate(data, !!! list_names)
# Reserverd highcharts names (#241)
if(has_name(data, "series"))
#old <- "seriess"
#new <- "series"
data <- dplyr::rename(data, "seriess" = "series")
if(drop)
newv <- rlang::syms(newv)
data <- dplyr::select(data, !!! newv)
data
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that. Let me know when it's ready for testing again :)
@tungmilan made the switch you suggested -- let me know how your tests go! |
@cnolanminich : great! will do it later today |
@cnolanminich : I've finished the testing. Everything looks good. I think it's ready :) |
@jbkunst let me know if there's anything else you need on my end for this to be merged |
Ok! @cnolanminich @tungmilan let's merge guys. Thanks you all! |
great job guys. much appreciated! @cnolanminich @tungmilan @jbkunst |
Add tidyeval to `hcaes` and `mutate_mapping`, thnks to @cnolanminich
Fixes #420
Updated primarily based on this ggplot2 commit that used rlang in the definition of
aes()
.Tested against the examples with ggplot2 2.2.1.9000’, and ran a couple of our apps that use both
ggplot2
andhighcharter