Fix NSE issues #2569
Fix NSE issues #2569
Conversation
Some IDEs don't execute tests in new process
So hybrid evaluation can detect these functions Closes tidyverse#2273
Now that literals are not rewrapped
R/count-tally.R
Outdated
| sum(!! wt, na.rm = TRUE) | ||
| } | ||
| ) | ||
| if (is_empty_quosure(wt) || is_null(f_rhs(wt))) { |
There was a problem hiding this comment.
quo_is_missing(wt) || quo_is_empty(wt)?
There was a problem hiding this comment.
that's not the naming scheme we've followed so far. It's not clear what is best to do in these cases. Same issue occurs with set_ and mut_ functions. Currently rlang does not type-prefix all these functions.
There was a problem hiding this comment.
I thought we had been pretty consistent with a type prefix. I think it would help here.
There was a problem hiding this comment.
I think these would be the very first predicates prefixed with a type in rlang.
This is linked to the issue of automatically taking the RHS in all language predicates. E.g. is_symbol() is shortcut for is_symbol(f_rhs(x)). I think this works mostly fine but is confusing because not all rlang predicates behave this way. An alternative would be a set of quosure-specific predicates. Then it probably makes sense to prefix all of them with quo, e.g. quo_is_null(), quo_is_symbol().
There was a problem hiding this comment.
What if we combined them both into quo_is_empty()? Then it's a specific definition of emptiness that applies primarily to quosures.
There was a problem hiding this comment.
hmm I think we should be consistent with base R, otherwise this will be confusing.
There was a problem hiding this comment.
hmm I think we should be consistent with base R, otherwise this will be confusing.
maybe is_empty_quosure() needs to be quo_is_missing() then?
There was a problem hiding this comment.
Maybe it's just awkwardness in the design of count(): most functions would either check for missingness or NULL, not both. And missingness is probably what needs a special predicate.
There was a problem hiding this comment.
these functions are definitely awkardly designed, but at least they pointed our attention to missing quosures and literal rewrapping :)
R/count-tally.R
Outdated
| } | ||
| ) | ||
| if (is_empty_quosure(wt) || is_null(f_rhs(wt))) { | ||
| n <- ~n() |
There was a problem hiding this comment.
I think we should use quo() here, just to be consistent
There was a problem hiding this comment.
I was thinking we should only use quo() when we need to unquote something, so this provides additional intent in the code. This rule would not apply in introductory texts.
There was a problem hiding this comment.
I think simplicity wins over the minor signal loss.
R/funs.R
Outdated
| names(funs) <- names2(funs) | ||
| missing_names <- names(funs) == "" | ||
| default_names <- map_chr(funs[missing_names], function(dot) { | ||
| quo_text(node_car(f_rhs(dot))) |
There was a problem hiding this comment.
I think we want to allow unquoting literal functions there.
There was a problem hiding this comment.
Isn't this line just determining the default name?
There was a problem hiding this comment.
Yes but quo_name() will abort with literal inputs.
There was a problem hiding this comment.
oh that makes sense, expr_name() needs to be changed then
|
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/ |
order_by(),top_n(),sample_n()etc now use tidyeval. Fixes Several functions (with prepending) won't work if you don't load dplyr #2297.funs()now handles namespaced functions. Fixes pkg::fun doesn't work in dplyr::funs #2089.depends on an rlang update where literals are not rewrapped on capture → simpler count/tally implementation. Also the last arguments of captured dots are now gobbled by default, which fixes Should dplyr commands gobble empty last argument? #1039
bare functions are now transformed to a call in colwise functions. This allows the hybrid evaluator to kick in, and this slightly simplifies the lazy table backend. The performance issue of colwise functions is now back to where it was at the time of
mutate_each()andsummarise_each(): still slow with many columns, but much better than reported insummarise_allperformance #2273.colwise-dplyr-purrr-nogroups.pdf