Call helper #4226
Call helper #4226
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4226 +/- ##
==========================================
+ Coverage 99.61% 99.61% +<.01%
==========================================
Files 73 72 -1
Lines 13884 13892 +8
==========================================
+ Hits 13830 13838 +8
Misses 54 54
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
sub_in_funs is misleading, function is a valid output input in some cases like .SDcols, thus better name would be sub_in_calls @MichaelChirico name is misleading because it tests for function calls and not functions names
Sorry, not quite following, is this in reference to a specific place in the diff? or speaking more generally? |
|
Have made a small edit to Would be interesting to know how |
src/chmatch.c
Outdated
| if (!isString(x) && !isNull(x)) { | ||
| // for use in sub_*_funs from nse_utils.R | ||
| if (TYPEOF(x) == SYMSXP) { | ||
| return chmatchMain(coerceVector(x, STRSXP), table, nomatch, chin, chmatchdup); |
There was a problem hiding this comment.
Do we need a PROTECT() here for the new coerceVector object?
| @@ -1646,14 +1646,14 @@ replace_dot_alias = function(e) { | |||
| if (dotN(q)) return(TRUE) # For #5760 | |||
| # run GForce for simple f(x) calls and f(x, na.rm = TRUE)-like calls where x is a column of .SD | |||
| # is.symbol() is for #1369, #1974 and #2949 | |||
| if (!(is.call(q) && is.symbol(q[[1L]]) && is.symbol(q[[2L]]) && (q1c <- as.character(q[[1L]])) %chin% gfuns)) return(FALSE) | |||
There was a problem hiding this comment.
@mattdowle I believe here, note the comment about is.symbol
There was a problem hiding this comment.
@MichaelChirico Thanks. This is a convoluted one as it's inside !() so it's really (!is.call(q) || !is.symbol(q[[1L]])) && .... Haven't looked as the issues yet but it seems different use case than sub_is_fun. Btw, what about a name like is_call_to instead of sub_is_fun. The one function is_call_to(e, f) could accept length(f) 1 or >1 so we don't have to use sub_in_funs instead.
|
Function is a valid input for |
|
Great idea. This does simplify many internal lines very nicely and avoids expression repetition to reduce risk. I just simplified it down to one |
|
Maybe the new file in future then. FWIW, I'm not a fan of the NSE name. The term "non-standard" makes it sound non-desirable to me (since everyone seems these days to want to do what everyone else does; the standard). I always thought of it as delayed or lazy evaluation, as opposed to eager evaluation, and one of the great things about R. |
As we can see here, we have been using
is.call(x) && x[[1L] == f-alike syntax a lot internally, and not always consistently.This PR adds some helpers to
nse_utils.Rinstead of adding these inutils.R)feedback welcome