Sg/display func #233
Sg/display func #233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
=======================================
Coverage 75.38% 75.38%
=======================================
Files 5 5
Lines 459 459
=======================================
Hits 346 346
Misses 113 113 |
sebasguts
force-pushed
the
sg/DisplayFunc
branch
3 times, most recently
from
661f499 to
35eee8c
Compare
| function Display(x::MPtr) | ||
| local_var = "julia_gap_display_tmp" | ||
| AssignGlobalVariable(local_var,x) | ||
| xx = EvalStringEx("Display($local_var);")[1] |
There was a problem hiding this comment.
Huh, it shouldn't be necessary to jump through such hoops. With that, I don't mean there is a way to avoid it right now (sadly, I fear there is none, so your hack is needed for now). But rather, that there should be a way. At least for Print, you can use PrintTo to redirect to a file or stream. So perhaps we need to add ViewTo and DisplayTo` functions to GAP. And/or provide a more general mechanism to redirect the output.
There was a problem hiding this comment.
The GAP package GAPDoc provides functions StringPrint and StringView (based on PrintTo1) which create strings for a given object that describe the Print and View output for this object, respectively.
An analogous StringDisplay could be provided, and these functions could be taken for producing the strings needed by Julia and Jupyter notebooks and the like, at least temporarily.
It is not obvious for me which strings one really wants for a given GAP object.
The above functions create strings with line breaks according to the current line length.
In the case of Display, I would say that this is fine:
Those objects for which Display is most interesting require a two-dimensional formatting,
and the line length is needed to define the context.
In the case of View and Print, one can alternatively produce one string with line breaking hints \< and \> for a given object, and postpone the actual formatting, in the sense of responsive design.
(After the recent change #3440 in gap-system/gap, I am more optimistic that ViewString can work as a long-term solution.)
There was a problem hiding this comment.
I would honestly prefer to use something like DisplayString, which is already defined in GAP, but does not really provide me with the strings I really need.
This function is for a very special case only, i.e., using it from the Jupyter Notebook with Julia kernel. Of course, we can think about a different solution, and implement all kinds of backend for it in GAP, or even cleanup the whole output mess in GAP. But imho this function has a very huge advantage over all the stuff you propose: It works right now.
So, while all your comments are totally valid, and this function is really not nicely designed or anything, I would suggest we merge it now, and then later go through the whole shebang of doing what you propose. I would happily write a DisplayTo function for GAP, but keep in mind that this might be a non-trivial task.
There was a problem hiding this comment.
@fingolfin I have to apologize a bit for my whitty comment. I did not read that you also probably not think there is a better ad-hoc solution in GAP. So okay, I think we overall agree that this is horrible and should not be done this way, but it is the only way to do it right now.
On the other hand, as mentioned above, the function is really for a very specific case, and once a better solution comes around, we can just change it.
So overall, I am sorry that I actually read it in the wrong way, namely that it should be done differently. But now reading the comment again, it seems like we both agree on that stuff.
|
I updated this PR to the final version of gap-system/gap#3438 |
|
Added a comment and merged your suggestions. Thank you :) |
Currently based on
#232 and gap-system/gap#3438