feat: add $meta$show_graph()
#64
Conversation
etiennebacher
force-pushed
the
show_graph
branch
from
7915bfc to
ace665f
Compare
Maybe pola-rs/r-polars#928 (comment)? Related upstream issue: pola-rs/polars#20631 |
|
In the At the same time, if we keep |
|
My concern is not the dependency but the API. In my opinion, we should separate functions rather than controlling the type of output with arguments. |
|
|
||
| fn meta_tree_format(&self) -> Result<Sexp> { | ||
| self.compute_tree_format(false) | ||
| } | ||
|
|
||
| fn meta_show_graph(&self) -> Result<Sexp> { | ||
| self.compute_tree_format(true) | ||
| } |
There was a problem hiding this comment.
There is no need to add these functions; just call <expr>$compute_tree_format(<bool>) from the R side.
| #' Note that Graphviz must be installed to render the visualization (if not | ||
| #' already present, you can download it here: | ||
| #' `<https://graphviz.org/download>`_). |
There was a problem hiding this comment.
I think this explanation is incorrect; Graphviz is not required.
I'd agree if we were building our own API but here we're following an existing one that faces the same challenges. I think we could instead talk about this upstream, especially with the (I can add a comment on the mermaid PR if you agree with this suggestion) |
I think we talked before about whether we should include the options to print / save the graph using external packages. I don't remember the decision, but no problem to add / remove functionalities here.