Use TableTraits.isiterabletable in rows and columns #89
Conversation
Otherwise we misdetect some table types which do not define `Base.iterable`, like `ExcelFile`. Add a new test for this pattern.
nalimilan
force-pushed
the
nl/isiterabletable
branch
from
fd17990
to
72f0cca
Compare
|
Thanks! |
| @@ -58,7 +58,7 @@ function rows(x::T) where {T} | |||
| if columnaccess(T) | |||
| cols = columns(x) | |||
| return RowIterator(cols, rowcount(cols)) | |||
| elseif Base.isiterable(T) | |||
| elseif TableTraits.isiterabletable(x) === true || Base.isiterable(T) | |||
There was a problem hiding this comment.
I think we could further simplify this: TableTraits.isiterabletable(x) will return true if Base.isiterable(T) is true, so we don't need that second test. Also, I think we also want to take this branch if TableTraits.isiterabletable(x)===missing, right? So I think the test should probably read elseif TableTraits.isiterabletable(x) !== false.
But having said this, this PR is clearly fixing a bug already, so thanks for opening it!
There was a problem hiding this comment.
You're clearly the expert here, so feel free to open a PR to improve this.
There was a problem hiding this comment.
Will do once I'm done with my current travel stint. @quinnj could you tag a bug fix release that includes this PR soon? Tables v0.2.0 broke the entire file IO story in Queryverse, and this PR fixes that.
Otherwise we misdetect some table types which do not define
Base.iterable, likeExcelFile.Add a new test for this pattern.
Fixes queryverse/ExcelFiles.jl#33, which appeared after #84.
CI fails due to version incompatibilities, not sure why (EDIT: that's #88).