Re: [PATCH] Colorized REPL

[Daniel Hartwig]

Hello again

Some comments in addition to Ludo's below.  I have not inspected the
code of your latest submission thoroughly, but enough to agree that
there are many stylistic and algorithmic issues.  I will probably not
be looking in to it any more, and remain a satisfied user of
emacs+geiser.

I still think that this data colourizing or whatever is the domain of
third-party packages, rather than something to include in Guile
proper.


> string-in-color

colorize-string is a much nicer name.

> enable-color-test disable-color-test

These should not be exported.

> colorize

This is the most useful procedure in the module, it should really be exported.


The default colour scheme is far too aggressive and not to my taste at
all.  The focus should be on highlighting the structure (i.e. syntax),
but that is hard to spot when practically every data type has it's own
tweaked colours.  Highlighting is subtle, only a very few conditions
should change the colour: strings and parens are great, but please
leave numbers in whatever the current colour is.

That said, a colour scheme for testing should probably be quite
aggressive, to the point of giving every condition it's own unique set
of attributes.

Also, the “/” in “1/2” appears as a different colour, why?!


On 9 January 2013 18:17, Nala Ginrut <address@hidden> wrote:
> On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
>> Nala Ginrut <address@hidden> skribis:
>> > (activate-colorized)
>>
>> I did that, and actually had to jump into a recursive REPL to see it in
>> effect.  Would be nice to fix it.
>>
>
> Well, I'm not sure what's the mean of 'recursive REPL'?

Starting one REPL inside another.  The updated activate-colorized only
sets default REPL options and does not take care to update the current
instance if one is already active.  So, if an REPL is already running,
one has to do (activate-colorized) followed by starting a new REPL.

>> Below is a rough review.  There are many stylistic issues IMO, such as
>> the lack of proper docstrings and comments, use of conventions that are
>> uncommon in Guile (like (define foo (lambda (arg) ...)),
>> *variable-with-stars*, hanging parentheses, etc.), sometimes weird
>> indentation, and use of tabs.

Procedure arguments “data” which should rather be “obj”.

>>
>> Overall it’s essentially a new implementation of write/display, so I’m a
>> bit concerned about keeping it in sync with the other one.

Yes this is quite concerning.  Having less implementations is
certainly preferable to having more.

The current output is broken for some data types:

scheme@(ice-9 colorized)> (colorize-it #f32(0 1 2))
#vu8(0 0 0 0 0 0 128 63 0 0 0 64)
scheme@(ice-9 colorized)> (colorize-it #u8(0 1 2))
#vu8(0 1 2)
scheme@(ice-9 colorized)> (write #u8(0 1 2)) (newline)
#u8(0 1 2)

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

>> Could you
>> add test cases that compare the output of both, for instance using a
>> helper procedure that dismisses ANSI escapes?

You have provided:

> (define color-it-test
>   (lambda (color str control)
>     str))

rather, you want to write a procedure that takes a string with ANSI
code sequences embedded and removes the ANSI codes so that only plain
text remains.  That plain text can then be compared with the output
from write/display.

 (define (remove-ansi-codes str) …)

then use that in the test-cases such as:

 (define (compare-write-and-colorize obj)
   (string= (with-output-to-string
             (lambda () (write obj)))
            (remove-ansi-codes
             (with-output-to-string
              (lambda () (colorize obj))))))

but structure your test cases as per the existing test-suite.

> OK, I added a #:test in 'colorize' and a color-it-test for it.
> But I know little about the test case of Guile, anyone point me out?

See the existing tests filed under test-suite/tests.

>> Would it make sense to define a new type for colors?  Like:
>>
>>   (define-record-type <color>
>>     (color foreground background attribute)
>>     color?
>>     ...)
>>
>>   (define light-cyan
>>     (color x y z))
>>
>
> Actually, I did similar things (though without record-type), but I was
> suggested use the *color-list* implementation from (ansi term) from
> guile-lib. hmm... ;-)
> Anyway, I think that implementation is not so clear, and it mixed
> 'colors' and 'controls' together...

Right, lists are more natural for specifying these sets of attributes,
which could be any combination of foreground, background, and/or
something other.  (ansi term) module sets a very respectable example.

>> > +(define color-it
>> > +  (lambda (cs)
>> > +    (let* ((str (color-scheme-str cs))
>> > +      (color (color-scheme-color cs))
>> > +      (control (color-scheme-control cs)))
>> > +      (color-it-inner color str control))))
>>
>> This is somewhat confusing: I’d expect (color-it str cs), but instead
>> the string to be printed is embedded in the “color scheme”.
>>
>
> It's a convenient way to enclose string into 'color-scheme', since the
> string could be used later.

I agree with Ludo, the string and color scheme are not so related.
This design choice adds confusion to the rest of the module.

>
>> > +(define (backspace port)
>> > +  (seek port -1 SEEK_CUR))
>>
>> What about non-seekable ports?  Could it be avoided altogether?
>>
>
> But I think the 'port' parameter in 'call-with-output-string' is always
> seekable, isn't it? The 'port' here is not a generic port.

Regardless, it is poor style to produce something only to subsequently
scrub it out.  Code does this:

+          (for-each (lambda (x) (colorize x port) (space port)) data)
+          (backspace port)  ;; remove the redundant space

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

 (display (string-join (map colorize data) " ") port)

or, perhaps more efficiently, this:

 (format port "~{~a~^ ~}" (map colorize data))

>> > +(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.

Also, this colours the vectag (such as “s16” or “u8”) for arrays, but
does not do the same for bytevectors.  This comes across as very
inconsistent, especially with two such values next to each other.
Just leave the array tags in a single colour.

>> > +(define *colorize-list*
>> > +  `((,integer? INTEGER ,color-integer (BLUE BOLD))
>> > +    (,char? CHAR ,color-char (YELLOW))
>>
>> Instead of a list, can you instead define a record for each token color
>> setting?
>>
>>   (define-record-type <token-color>
>>     (token-color name pred color-proc color)
>>     token-color?
>>     ...)
>>
>>   (define %token-colors
>>     `(,(token-color 'integer integer? color-integer '(blue bold))
>>       ...))
>>
>
> Hmm...if it's unnecessary, I prefer be lazy...
>
>> > +(define type-checker

>>
>> Using call/cc here is fun but excessively bad-style.  :-)
>>
>> Try something like:
>>
>>   (or (any ... (current-custom-colorized))
>>       (any ... %token-colors)
>>       (token-color 'unknown (const #t) color-unknown '(white)))
>>
>
> But in this context, I need a finder which could return the result, not
> just predicate true/false, 'any' seems can't provide that.

You might want to reread the definition of “any”.

Best wishes.