don't trust all.vars when its arguments contains get/eval #4982
Conversation
… get/eval and siblings.
Codecov Report
@@ Coverage Diff @@
## master #4982 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 73 73
Lines 14612 14615 +3
=======================================
+ Hits 14532 14535 +3
Misses 80 80
Continue to review full report at Codecov.
|
|
Note this solves #4873 too. |
mattdowle
changed the title
|
news item and addition to contributor list in 531be37 |
|
@OfekShilon Many thanks. I invited you to be project member so amongst other things you can create branches in main repo in future. The invite is a button in your profile or projects page in github that you have to click to accept. Thanks again and welcome! |
|
There were no unit test for the second closed issue so I added them in follow up PR #4985 |
|
@mattdowle Thank you very much, I'll do my best to contribute. If I understand correctly - I'm not allowed to merge PRs, right? Also, what is the right place to ask such questions? (probably not in a PR comments) |
|
Just me (and Arun) who can merge currently, but that may expand to Jan and Michael in future. |
| allbyvars = intersect(all.vars(bysub), names_x) | ||
| # Fix 4981: when the 'by' expression includes get/mget/eval, all.vars | ||
| # cannot be trusted to infer all used columns | ||
| bysub.elems <- rapply(as.list(bysub), as.character) |
There was a problem hiding this comment.
This is failing R 3.1.0 with :
> af[, mean(a), by = "itime"]
Error in .Primitive("as.character")(list, ...) :
cannot coerce type 'builtin' to vector of type 'character'
Calls: [ -> [.data.table -> rapply
Execution halted
will fix ...
Closes #4873
Closes #4981
There are at least 2 other places in
[.data.tablethat test for inclusion ofgetet.al. in an expression - I think they should use rapply too, but didn't touch it.