Closes #1678; dimnames.data.table properly uses inherits to check data.frame awareness #1679
Conversation
Current coverage is 90.68%@@ master #1679 diff @@
==========================================
Files 55 55
Lines 10144 10149 +5
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 9202 9203 +1
- Misses 942 946 +4
Partials 0 0
|
| @@ -2022,7 +2022,12 @@ as.list.data.table <- function(x, ...) { | |||
|
|
|||
| dimnames.data.table <- function(x) { | |||
| if (!cedta()) { | |||
| if (!identical(class(x),c("data.table","data.frame"))) stop("data.table inherits from data.frame (from v1.5) but this data.table does not. Has it been created manually (e.g. by using 'structure' rather than 'data.table') or saved to disk using a prior version of data.table? The correct class is c('data.table','data.frame').") | |||
| if (!inherits(x, "data.frame")) | |||
There was a problem hiding this comment.
this should be if (!is.data.table(x)). Also could you please leave the error message as is.. no need for unnecessary paste.
There was a problem hiding this comment.
@arunsrinivasan I think the fact that it gets here in the first place (unless a user invokes data.table:::dimnames.data.table directly -- and anyone doing that better know what they're doing) implies that inherits(x, "data.table") is TRUE, since something dispatched to a data.table method. Am I mistaken?
This is why I think we can ditch the test altogether -- seems this worry about x being data.table but not data.frame is pretty obsolete.
If we do test, we should test for data.frame so we're sure dimnames.data.frame is proper to dispatch.
There was a problem hiding this comment.
Oh I see what you mean. Sounds good. Could you please make that as the only change? Thanks.
It's meant to handle cases where one does:
x = list(1:2, 3:4)
setattr(x, 'class', "data.table")Rare, but a possibility.
There was a problem hiding this comment.
@arunsrinivasan you mean to remove the test, or to keep inherits(x, "data.frame") but remove the paste?
There was a problem hiding this comment.
@arunsrinivasan done
…a.frame awareness honing bug fix update un-paste
Wasn't sure how to add proper tests, since we need to invoke
!cedta()to get the original error.