groupingsets(): introduce and make use of the enclos argument#6899
groupingsets(): introduce and make use of the enclos argument#6899jangorecki merged 3 commits intomasterfrom
groupingsets(): introduce and make use of the enclos argument#6899Conversation
When forwarding NSE arguments via jj=, also forward the environment containing the symbols they may be referencing. Now groupingsets() substitutes all the local arguments into the call and then evaluates x[, jj, ...] in the specified environment, making it possible for jj to refer to local symbols without confusing them to variables belonging to groupingsets() or its caller. To avoid cedta() problems, set .datatable.aware = TRUE in the environment where the call is evaluated. (A column named .datatable.aware would be shadowed. So it goes.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6899 +/- ##
=========================================
Coverage ? 98.50%
=========================================
Files ? 79
Lines ? 14759
Branches ? 0
=========================================
Hits ? 14538
Misses ? 221
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Generated via commit 44f5f09 Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
Basically makes sense to me, though I'm not all that fluent with I am taking @jangorecki 's removal as reviewer to be "no opposition" to the extended API. |
|
Go ahead, will look and understand later on... |
|
This does need fixing, right? Do we want to discourage referring to variables outside the |
|
Yes, definitely. Thank you Ivan. |
|
@aitap news entry needs reference to issue or PR |
|
We could potentially add a lint rule for this, but it requires some more careful approach:
|
|
I would leave the old ones as is and just use it now. We can test some linters but then maybe rather to switch then on after next NEWS.n file archive. |

When forwarding the
jargument for non-standard evaluation viajj=, also forward the environment containing the symbols that it may be referencing. Nowgroupingsets()substitutes all the local arguments into thex[, jj, ...]call and then evaluates it in the specified environment, making it possible forjjto refer to local symbols without confusing them to variables belonging togroupingsets()or its caller.To avoid
cedta()problems, set.datatable.aware = TRUEin the environment where the call is evaluated. (A column named.datatable.awarewould be shadowed. So it goes.)An alternative solution would give the
enclos=argument to[.data.tableinstead of evaluating the[call insideenclos, but that would require carefully replacing a lot ofparent.frame()calls. Also,[.data.tablealready has anenv=argument, which operates on the expression directly, not provides an enclosing environment.Another alternative solution could construct Brodie Gaslam's DIY quosure, but
[.data.tabledoesn't seem to "see" inside such calls (for example, to replace the.(...)special form withlist(...)) when given one as thejargument and I didn't investigate it further.Fixes: #5560