-
Notifications
You must be signed in to change notification settings - Fork 990
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
remove use of rbindlist(..., use.names=FALSE, fill=TRUE) in merge #5857
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5857 +/- ##
=======================================
Coverage 97.47% 97.47%
=======================================
Files 80 80
Lines 14823 14829 +6
=======================================
+ Hits 14448 14454 +6
Misses 375 375 ☔ View full report in Codecov by Sentry. |
I would add this example from #5309 as well -- surprising for the PR closing an issue on t2 = data.table(a = NA_real_, b = NA_real_, c = NA_real_, d= as.Date(NA))
t1 = data.table(a = 1.1, b = 1.1, c = 1.1, d = as.Date('2021-10-05'), e = as.POSIXct("2021-10-06 13:58:00 UTC"))
r2 = rbind(t1,t2, fill = T, use.names = F) Or maybe:
Implies that a test already exists? If so please help point to it. But #5444 summary mentions R crashing, and #5309 was a normal R error, so I'm still not sure of the connection. |
Added the test. Problem on both #5309 and #5444 was that non-existing columns were compared to existing columns due to fill. In #5444 this even led to a segfault ( |
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.
LGTM, thanks! Leaving unmerge in case Jan has anything else first, but feel free to merge & we can handle other issues in follow-up if need be.
* add fix #5309 * fix test numbering * add rbind for ITime * more tests * add merge tests * add AsIs #4934 * add news * news typo * add ignore.attr argument * fix news * change arguments of registered rbindlist * add attribute to usage * move nanotime tests * adjust test numbering * add test coverage * prohibit NA for ignore.att * move news * finish todo of #5857 * Update NEWS.md Co-authored-by: Michael Chirico <[email protected]> * update comment * update doc for ignore.attr * fix nit ignoreattr * fix test consistency * remove setnames * update asis test to use rbindlist * update test comments * update NEWS num * NEWS wording * more NEWS wording * template message for i18n * simplify condition (C boolean --> no NA to worry about) * && not & * correct error message --------- Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]>
Closes #5309 (error part in
rbind
is duplicate of #5444)Pre #5446