Fix diffing empty DataTables #1097
Conversation
There was a problem hiding this comment.
@botandrose Let me know what you think of the changes I suggested, if you disagree we can talk about it.
| return ensure_2d(cols.transpose), ensure_2d((matched_cols + unmatched_cols).transpose) | ||
| end | ||
|
|
||
| def ensure_2d(array) #:nodoc: |
There was a problem hiding this comment.
I'm wondering if we should make this method private.
There was a problem hiding this comment.
It's already protected... is that good enough?
There was a problem hiding this comment.
Oh yeah, that's great. I didn't see that.
| t1 = DataTable.from([[]]) | ||
| t2 = DataTable.from([[]]) | ||
| t1.diff!(t2) | ||
| end |
There was a problem hiding this comment.
Maybe use something like
expect{ t1.diff!(t2) }.not_to raise_error
would be better?
There was a problem hiding this comment.
Technically redundant, but more explicitly conveys the intent. I like! 👍
There was a problem hiding this comment.
Cool, if you could just change that and push it up I'll merge :) Thanks!
botandrose
force-pushed
the
fix_diffing_empty_datatables
branch
from
ae1c759 to
cc61811
Compare
|
Okay! Pushed, and all 💚 here. Thank you for your time and attention! I'll be submitting fixes for one or two more DataTable-related bugs over the coming days, BTW. |
|
Thank you @botandrose - we really appreciate your help with the project! |
|
Thanks for handling it @danascheider 👌 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes the following:
See #1096 for much more details.
Details
Added a private
#ensure_2dmethod to DataTable, and used it internally in#pad_and_matchto make sure we're always dealing with an array of arrays, which the code had previously been assuming.Motivation and Context
Fixes #1096
How Has This Been Tested?
Added unit test to expose bug, then wrote patch to fix it.
Types of changes
Checklist: