Re: [PATCH] Colorized REPL

[Daniel Hartwig]

On 11 January 2013 14:29, Nala Ginrut <address@hidden> wrote:
> On Thu, 2013-01-10 at 16:19 +0800, Daniel Hartwig wrote:
> I changed these:
> string-in-color => colorize-string
> display-string-in-color => colorized-display
>
> What do you think?

Nicer anway.

>> Also, the “/” in “1/2” appears as a different colour, why?!
>>
>
> It's more conspicuous for the users. I asked some guys, they like it.
>

I see.

>
> But how to check if a REPL is already running?

I believe there was some fluid referenced by a previous version of
activate-colorized.

>> Procedure arguments “data” which should rather be “obj”.
>>
>
> Colorize an 'obj' is strange, I think 'data' is better.
>

Not strange at all in the context of Scheme where this is a well
established convention.  “obj” literally means “any object”, talking
about Scheme objects (not GOOPS).  “data” is not commonly used and
when it is, typically has a very specific meaning that does not
include “any object”.  To read a procedure definition that uses the
name “obj” I can immediately tell what types of values it expects
(anything!), whereas “data” carries no particular hints at all.

In any Scheme documentation, such as the Guile manual or any Revised*
Report on Scheme, there is almost exclusive use of the name “obj” for
arguments like this.  See the R5RS section “Entry format”.

> But what's the expected result of "(write #u8(0 1 2)) (newline)"?

As I wrote: #u8(0 1 2).  I provided that line to demonstrate that
colorize-it was producing different output.

>
>> The chance of subtle problems with other data types is high, both now
>> and in the future after any current problems are corrected.
>>
>
> Any code contains potential bugs.

Yes.

> I don't think people should use (ice-9 colorize) in their serious
> program, it's just a tool for newbies to learn Guile more friendly. Even
> 'colorize-string', it's the co-product of that, and it just output a
> string in color, simple enough to avoid dangerous.

Regardless of the purpose, if it is shipped as part of Guile then it
will be used.  It's output must be reliable.

Keep in mind that you can not anticipate everything people will do
with code.  It takes on it's own life and you can only smile and watch
it play :-)

>
> But it's inefficient if I remove ansi-code each time after it's
> generated. I prefer output the plain string on the fly with enable test
> option.

Test cases do not have to be efficient, they must prove the real code.
 By replacing the innermost procedure—colorize-the-string—with a
testing variant you are no longer testing the real code, but something
else.

>> I agree with Ludo, the string and color scheme are not so related.
>> This design choice adds confusion to the rest of the module.
>>
>
> OK, I think it's an absolute design, fixed.

I suppose the original comments were not so clear.  It is not only the
string but other members such as “data” that do not fit the concept of
“colour scheme”.  Anyway, given that it is an internal type there is
not much point to restructuring it all, except for pedantics.


>> when, if “colorize” produced strings, it could do something like this:
>>

> Nice~I'm in the first way, and added a helper function '->cstr' to
> generate color string result for any type.
> A good hack I like it~

Indeed.  The new loops are certainly an improvement.

Personally I would have avoided call-with-output-string in ->cstr and
other low-level procedures, since now there are several calls in and
out of string ports.  I suppose that is another early design decision
that is not worth the effort to change.

>
>> >> > +(define color-array-inner
>>
>> >> Wow, this is hairy and heavyweight.
>>
>> > Yes, but the aim of colorized-REPL is to show more friendly UI to the
>> > users, it dropped up some efficiency designs.
>>
>> Disagree.  It is more difficult to read the array tag with all the
>> colour changes.  Breaking apart elements to this level is extreme,
>> prone to errors, and poorly maintainable.  All that for a very
>> questionable gain in “friendly UI”.  Keep things simple, at least
>> until you have a smooth module without issues.
>>
>
> Even if I don't break apart it, it's inefficient too, I have to convert
> it to string then output the prefix-part.

You could inspect the array and generate the tag yourself.  This would
not explode on very large data structures.  But, as you go on to say …

> As I said, nobody will use it in one's serious program, since it's only
> about REPL. Who care about the REPL running speed? Users like a more
> friendly REPL UI not a quick REPL since it's useless in a released
> program but for test/debug. Except for 'colorized-string', but it's a
> simple function which is safe and fast.

Indeed, with this outlook there is little point to such alternative
treatment, and the easiest thing to do is to rely on the existing
procedures to produce the correct output.

However I will say that an efficient and flexible implementation would
certainly be useful in applications outside of the developers REPL.
Applications display data, and often contain REPL and equivalent. :-)

> Please review it:
> https://github.com/NalaGinrut/guile-colorized/tree/upstream
>

This:

 (define colorize-the-string
   (lambda (color str control)
     (string-append "\x1b[" (generate-color color) "m" str
       "\x1b[" (generate-color control) "m")))

Why move "\x1b[" and "m" out of generate-color here?  Have that
procedure procedure the /complete/ escape sequence and it is much
neater.

Regards