Repository navigation
move helper functions outside of [ - #7935
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/a3_callenv #7935 +/- ##
==================================================
- Coverage 99.09% 99.09% -0.01%
==================================================
Files 88 88
Lines 17431 17422 -9
==================================================
- Hits 17273 17264 -9
Misses 158 158 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
94d7529 to
4821512
Compare
4821512 to
296435e
Compare
|
No obvious timing issues in HEAD=brk/a4_move_helpers (Plot image upload to picrd.com failed; download the artifact below to view plots) Generated via commit b7fe976 Download link for the artifact containing the test results: ↓ atime-results.zip
|
Functions defined inside [ capture its frame, so they block moving the code that uses them. Move them to internal top-level functions, named .dt_* so that conditions they raise still report the [.data.table call. No behaviour change.
296435e to
b7fe976
Compare
| jvnames[head(key_idx, missing_keys[1L] - 1L)] | ||
| } | ||
| shared_keys = get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) | ||
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) |
There was a problem hiding this comment.
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars = sdvars, key(x)) | |
| shared_keys = .dt_get_shared_keys(jsub, jvnames, sdvars, key(x)) |
| raise_condition = function(signal, message, classes, immediate=FALSE, appendLF=FALSE) { | ||
| obj = list(message=message, call=sys.call(sys.nframe()-2L)) | ||
| n = sys.nframe() - 2L # the caller of stopf() etc. | ||
| # helpers extracted from [.data.table are named .dt_*; their conditions report the call of [.data.table (or whoever called them) |
There was a problem hiding this comment.
Maybe they should be named .dtq_ (query) to distinguish them from other internal functions?
Another alternative to consider (especially w.r.t. potential performance drawbacks) would be to S3 them:
.zip_items = function(...)
class(.zip_items) = "dt_query_helper"And here:
while (inherits(sys.call(n), "dt_query_helper")) ...(I haven't tested if sys.call() retains the attribute).
At a higher level, is it really that much more beneficial to see "Error in [.data.table" than "Error in .zip_items"? The latter would appear to be more specific about which of the many steps in [ went awry; and beginner users can be "scared" by "[.data.table" too (they've never written [.data.table before -- where did it come from?). Would just using call.=FALSE suffice?
Also, be sure to evaluate the robustness of the proposed solution. I have memories of endless headaches trying to inspect the call stack and provide better errors in {bit64} where I wanted to know "Did a user call this exported S3 method directly, or did they get here from the generic?". I ultimately decided it was impossible to resolve in full generality. Edge cases to consider: DT() where [ gets masked by another name; tryCatch() / withCallingHandlers(); substitute().
Towards #852
Since this change consists of a lot code moving I would recommend reviewing it with
git diff brk/a4_move_helpers^! --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-changewhich displays the real changes nicely