-
Notifications
You must be signed in to change notification settings - Fork 991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix names_sd rev dep issue 6033 #6238
Conversation
Generated via commit 1024792 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 12 minutes and 0 seconds Time taken to run |
hi Cole thanks very much |
I'm not clear what's being deprecated, exactly -- the use of The fix here looks good but I wonder if we need to change anything in the longer-term, i.e., it looks fine to maintain this per my current understanding. |
That's a good point. Maybe deprecation is not needed. I was leaning that way because we need to add additional NSE to support a use-case that probably doesn't make sense. We don't see My inclination is that we should not support that use case - the caller should substitute prior to it getting to us. But I would also say it's easier just to keep the check to determine if the lhs is |
Isn't that what caused the revdep breakage though? Think I'm missing something |
#6033 revdep. Slowly deprecate in 1.17.0. Caller has given us `dt[, substitute(names(.SD))]` which means | ||
# jsub is actually substitute(names(.SD)) instead of just names(.SD) | ||
if (lhs %iscall% 'substitute') | ||
lhs = eval(lhs, parent.frame(), parent.frame()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to double-eval()
? Or can we do if (lhs %iscall% 'substitute') lhs = eval() else lhs = eval()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my understanding which is why I never understood how it used to work.
my_col = "a"
jsub = substitute(substitute(my_col))
eval(jsub, parent.frame(), parent.frame())
# my_col
eval(eval(jsub))
# [1] "a"
At the same time, there's this head scratcher:
eval(jsub, list(my_col = "b"), parent.frame())
# [1] "b"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Head scratcher indeed... OK I'm more of the opinion we shouldn't allow j=substitute(...)
, indeed it's hard to reason about what jsub = substitute(subsitute(...))
might do. We can start a deprecation process & see if there's a good use case, my guess is, the end user would be better served by some other approach though.
I was unclear, my bad. A lot of times we compare Regardless, it seems like we're on the same page. I'll make a follow-up issue after this is merged to start deprecating the allowance of |
Closes #6033 .
The root cause was that the caller would make a call as
dt[, substitute(col)]
. This meant ourjsub
would besubstitute(col)
which would only evaluate to the namecol
instead of the value ofcol
. I was unable to figure out why #4163 caused this issue to come to light.This is a quick fix meant for follow-up. The plan would be to:
I will start looking at rev deps again to fix.