Refactor helper function issue #6702 (Refactored duplicated logic in setDT and [,:=]) #6752
Conversation
nipun-gupta-3108
force-pushed
the
refactor-helper-function-issue-6702
branch
from
0be7133 to
97e1517
Compare
…com/nipun-gupta-3108/data.table into refactor-helper-function-issue-6702
.devcontainer/devcontainer.json
Outdated
| @@ -1,9 +1,10 @@ | |||
| { | |||
| "build": { "dockerfile": "r-devel-gcc/Dockerfile", "context": ".." }, | |||
| "build": { "dockerfile": "Dockerfile", "context": "../.." }, | |||
There was a problem hiding this comment.
it looks like you've still got some edits from earlier #6747 included here -- please revert, for example:
git checkout master -- .devcontainer/devcontainer.json
|
Hi MichaelChirico, I'm encountering a test failure in test-coverage.yaml related to the pull request. The test is failing with the following error: Error in test.data.table(): Failed in 14.0s elapsed (14.6s cpu) after test 1035.32 before the next test() call in /home/runner/work/_temp/package/data.table/tests/tests.Rraw This issue is related to test 1035.32. Could you please help me understand why this test is failing? Thank you for your assistance! |
| @@ -2962,25 +2969,13 @@ setDT = function(x, keep.rownames=FALSE, key=NULL, check.names=FALSE) { | |||
| if (is.name(name)) { | |||
| name = as.character(name) | |||
| assign(name, x, parent.frame(), inherits=TRUE) | |||
| } else if (.is_simple_extraction(name)) { | |||
There was a problem hiding this comment.
please be aware that the underlying code has changed since what was permalinked in #6702. In particular note this .is_simple_extraction() helper function which was not present then. Please adapt.
| @@ -3295,7 +3295,7 @@ test(1034, as.data.table(x<-as.character(sample(letters, 5))), data.table(V1=x)) | |||
| test(1035.291, melt(dt, measure.vars=NA_integer_, id.vars=NULL), error="One or more values in 'measure.vars'") | |||
| test(1035.30, melt(dt, id.vars=NA_integer_), error="One or more values in 'id.vars'") | |||
| test(1035.31, melt(dt, measure.vars=NA_character_), error="One or more values in 'measure.vars'") | |||
| test(1035.32, melt(dt, id.vars=NA_character_), error="One or more values in 'id.vars'") | |||
| test(1035.32, melt(dt, id.vars=NA_character_), error="One or more values in 'id.vars' is invalid.") | |||
There was a problem hiding this comment.
this change is irrelevant
Hi MichaelChirico,
I’ve been working on refactoring the duplicated logic in the setDT function and the [,:=] operation as discussed in issue #6702.
Here’s what I’ve done so far:
Here’s the helper function I’ve created:
process_assignment <- function(name, x, parent_frame) {
k = eval(name[[2L]], parent_frame, parent_frame)
if (is.list(k)) {
origj = j = if (name[[1L]] == "$") as.character(name[[3L]]) else eval(name[[3L]], parent_frame, parent_frame)
if (is.character(j)) {
j = match(j, names(k))
if (is.na(j)) stopf("Item '%s' not found in names of input list", origj)
}
.Call(Csetlistelt, k, as.integer(j), x)
} else if (is.environment(k) && exists(as.character(name[[3L]]), k)) {
assign(as.character(name[[3L]]), x, k, inherits = FALSE)
}
}
I then replaced the original duplicated blocks with calls to this helper function in both places. For example:
In the setDT function:
} else if (name %iscall% c('$', '[[') && is.name(name[[2L]])) {
process_assignment(name, x, parent.frame())
}
Questions I’d Like Feedback On:
I’d appreciate your guidance on any further improvements or next steps.
Looking forward to your feedback!
Best regards,
Nipun Gupta