Repository navigation
move the cedta fallback outside of [ - #7936
ben-schwen wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## brk/a4_move_helpers #7936 +/- ##
====================================================
Coverage 99.09% 99.09%
====================================================
Files 88 88
Lines 17430 17431 +1
====================================================
+ Hits 17272 17273 +1
Misses 158 158 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2bf339a to
effc48f
Compare
(Plot image upload to picrd.com failed; download the artifact below to view plots) Generated via commit 91e1abd Download link for the artifact containing the test results: ↓ atime-results.zip
|
effc48f to
301e163
Compare
| # Throw a specific error message | ||
| stopf("[ was called on a data.table in an environment that is not data.table-aware (i.e. cedta()), but '%s' was used, implying the owner of this call really intended for data.table methods to be called. See vignette('datatable-importing') for details on properly importing data.table.", as.character(jsub[[1L]])) | ||
| } | ||
| Nargs = nargs - (!missing_drop) |
There was a problem hiding this comment.
Can we re-compute missing(drop) here? (and missing_i?)
Since this is an "unsupported" path it's fine to have a little overhead, I think it's preferable to have a minimal helper signature.
I also wonder if a do.call(`[.data.frame`, list(...)) approach could simplify(?) this arg-checking logic.
There was a problem hiding this comment.
Yes. We can recompute by also dropping drop=NULL default from [.
do.call(`[.data.frame`, list(...)) would still need to force i on list(DT, i, j) and would therefore error
301e163 to
afc353f
Compare
afc353f to
6f3ec8f
Compare
d7e8151 to
1e3eeac
Compare
1e3eeac to
3360bd8
Compare
No behaviour change.
3360bd8 to
91e1abd
Compare
Towards #852