Generic Tables.istable support #285
Conversation
|
Thanks for opening this @alyst, this will be a nice improvement.
|
alyst
force-pushed
the
all_tables_support
branch
from
d7c0254 to
f7ec21b
Compare
- implement the generic _has_trait(trait, obj), _obj_convert(trait, obj) mechanism for potential future extensions - QuartoNotebookWorkerDataFramesTablesExt: drop DataFrames dependency
alyst
force-pushed
the
all_tables_support
branch
from
f7ec21b to
e93c017
Compare
|
I've updated the PR, I have also added some comments/docstrings to clarify the ojs_define/ojs_convert mechanism. |
should lower the memory footprint.
alyst
force-pushed
the
all_tables_support
branch
from
e93c017 to
981aa7e
Compare
| # Internal function to check if the object supports a specific trait. | ||
| # QuartoNotebookWorker extensions that implement specific traits | ||
| # should override this function. | ||
| # The fallback implementation reports no trait support. | ||
| _has_trait(trait::Val, obj::Any) = false | ||
|
|
||
| # Trait-specific object conversion that is called when the object | ||
| # supports a specific trait (_has_trait(trait, obj) == true). | ||
| # QuartoNotebookWorker extensions that implement specific traits | ||
| # should override this function. | ||
| # The fallback implementation returns the object unchanged. | ||
| _ojs_convert(trait::Val, obj::Any) = obj | ||
|
|
||
| # The default object conversion function that is called when | ||
| # there is no more specific _ojs_convert(obj::T) | ||
| function _ojs_convert(obj::Any) | ||
| # check if the object implements any of the traits supported by extensions | ||
| if _has_trait(Val(:table), obj) | ||
| # if QuartoNotebookWorkerTablesExt is active and | ||
| # obj supports Tables.istable(obj) interface, do table-specific conversion | ||
| return _ojs_convert(Val(:table), obj) | ||
| else | ||
| # no specific trait support, no conversion by default | ||
| return obj | ||
| end | ||
| end |
There was a problem hiding this comment.
It really doesn't need to be a generic trait mechanism at the moment, just the simple dispatches outlined in #286 is sufficient and preferable from my point of view. Adding Val-based trait-like code is more complex that this needs to be.
There was a problem hiding this comment.
I've replaced trait-based approach with the one you suggest.
I have to say, whether _is_rows/__is_rows(nothing)/_rows/__rows(nothing) is more simple/clean than _has_trait(trait)/_ojs_convert(trait) is a matter of personal taste -- we are discussing just 4 lines of code.
What is more important, I think, is to document how that works, which I have tried to do for the functions around OJS.
At the moment QuartoNotebookRunner.jl supports ojs_define(df::DataFrame) via the extension that requires both DataFrames.jl and Tables.jl.
Actually, Tables.jl are not necessary to support DataFrames, one can just use eachrow(df) instead of Tables.rows(df).
This PR adds
_has_trait(trait, obj)/_ojs_convert(trait, obj)mechanism, and addsVal(:table)trait thatenables ojs_convert(df) for all types that support Tables.table interface (dropping
DataFramesdependency).The Tables.jl support is moved into the QuartoNotebookRunner directly:isrowtable(df)istable(df) is the generic way to check if the type supports Tables.jl, it is not possible to do it via the_ojs_convert()argument type constraints