-
Notifications
You must be signed in to change notification settings - Fork 985
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
rbindlist works for expression column #3811
Conversation
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.
I have no idea if anyone will find it useful, @Rdatatable/project-members?. IMO better to restrict data.table column type. Matrix cannot be a column directly, but has to be wrapped into list, same I think make sense for language objects.
inst/tests/tests.Rraw
Outdated
# expression columns in rbindlist #546 | ||
a <- data.table(c1 = 1, c2 = 'asd', c3 = expression(as.character(Sys.time()))) | ||
b <- data.table(c1 = 3, c2 = 'qwe', c3 = expression(as.character(Sys.time()+5))) | ||
test(2098, rbind(a,b)$c3, expression(as.character(Sys.time()), as.character(Sys.time()+5))) |
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.
I don't find this useful at all. The new expression doesn't even evaluate. I am against combining language objects in rbindlist. The useful scenario I had in the past was to have them in a list column, which at that time I was not aware (5 years ago).
Codecov Report
@@ Coverage Diff @@
## master #3811 +/- ##
==========================================
+ Coverage 99.41% 99.41% +<.01%
==========================================
Files 72 71 -1
Lines 13909 13266 -643
==========================================
- Hits 13828 13189 -639
+ Misses 81 77 -4
Continue to review full report at Codecov.
|
maybe just add unit tests modifying original example by wrapping expressions into lists? then PR will cover some existing use case. |
Marked as 1.13.0 as a way to exclude from 1.12.4, but still come back to it. |
codecov results aren't updating. log states that upload succeeded but then there's a timeout after that line. a rerun didn't fix it. proceeding to merge and we should see master's coverage update. |
Closes #546
Seems we shouldn't go too far down the path of allowing
expression
columns but the fix here is quite simple/easy to follow.