Re: [PATCH] Colorized REPL

[Ludovic Courtès]

Hi Nala,

Thanks for the update.

Nala Ginrut <address@hidden> skribis:

> On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:

[...]

>> Nala Ginrut <address@hidden> skribis:
>> 
>> > 1. colorized-REPL feature:
>> > Add two lines to your ~/.guile, to enable colorized-REPL feature: 
>> > (use-modules (ice-9 colorized))
>> > (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'? 

An inner REPL (info "(guile) Error Handling").

>> > 2. custom color scheme:
>> > Example:
>> > (add-color-scheme! `((,(lambda (data) 
>> >                         (and (number? data) (> data 10000)))
>> >                         MY-LONG-NUM ,color-it (RED))))
>> 
>> Nice.
>> 
>> > Add it to your ~/.guile or in your code at you wish.
>> > This feature is useful, because sometimes we need to test our program
>> > and output a colorful result for some monitoring purpose.
>> > PS: MY-LONG-NUM is an arbitrary name for your own color scheme, as you
>> > like.
>> 
>> Why is that name even needed?
>
> It's easy to debug or checkout the color-scheme info with the name.

Hmm, there’s other info that helps debugging, such as location info of
the procedure, but OK.

>> 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.
>> 
>> 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.  Could you
>> add test cases that compare the output of both, for instance using a
>> helper procedure that dismisses ANSI escapes?
>> 
>
> 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 under test-suite/tests/*.test.  There’s a small set of constructs to
express unit tests, such as ‘pass-if’.

>> Some other comments:
>> 
>> > +(define-module (ice-9 colorized)
>> > +  #:use-module (oop goops)
>> > +  #:use-module ((rnrs) #:select (bytevector->u8-list define-record-type
>> > +                          vector-for-each bytevector?))
>> 
>> Would be good to pull neither of these.
>> 
>> Could you use (srfi srfi-9) and (rnrs bytevectors) instead of the
>> latter?  For GOOPS, see below.
>> 
>
> record-type in r6rs is more convenient I think.

That’s not the question. ;-)  It doesn’t justify pulling in all of R6RS.

>> > +(define-record-type color-scheme
>> > +  (fields str data type color control method))
>> 
>> Could you comment this?  I’m not clear on what each field is.

Ping!

>> > +(define *color-list*
>> > +  `((CLEAR       .   "0")
>> > +    (RESET       .   "0")
>> > +    (BOLD        .   "1")
>> > +    (DARK        .   "2")
>> > +    (UNDERLINE   .   "4")
>> > +    (UNDERSCORE  .   "4")
>> > +    (BLINK       .   "5")
>> > +    (REVERSE     .   "6")
>> > +    (CONCEALED   .   "8")
>> > +    (BLACK       .  "30")
>> > +    (RED         .  "31")
>> > +    (GREEN       .  "32")
>> > +    (YELLOW      .  "33")
>> > +    (BLUE        .  "34")
>> > +    (MAGENTA     .  "35")
>> > +    (CYAN        .  "36")
>> > +    (WHITE       .  "37")
>> > +    (ON-BLACK    .  "40")
>> > +    (ON-RED      .  "41")
>> > +    (ON-GREEN    .  "42")
>> > +    (ON-YELLOW   .  "43")
>> > +    (ON-BLUE     .  "44")
>> > +    (ON-MAGENTA  .  "45")
>> > +    (ON-CYAN     .  "46")
>> > +    (ON-WHITE    .  "47")))
>> 
>> 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...

Which implementation?  I still think that using a disjoint type for
colors would be better than symbols.  Also, this is something part of
the API, so we can’t just leave it for later discussion.

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

Surely, but it mixes concerns.  Can you try to make sure ‘color-scheme’
objects are just that, color scheme?

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

String ports are seekable, right.  However, seeking here seems like a
hack: you could just as well adjust the printer to not write that extra
character instead of writing it and then seeking backwards.  WDYT?

>> > +    (if sign
>> > +  (display (color-it-inner color sign control) port)  ;; not array
>> > +  (display (color-array-inner cs) port) ;; array complecated coloring
>> > +  )))
>> 
>> Parentheses should be at the end of the previous line.
>> End-of-line comments should be introduced with a single semicolon.
>> 
>
> Fixed them all, comments convention & suspended right-paren. ;-)

There are still many conventions wrong, such as procedure definitions,
global variable names, missing docstrings, etc.  Could you try to fix
them as well?

>> > +(define color-array-inner
>> > +  (lambda (cs)
>> > +    (let* ((colors (color-scheme-color cs))
>> > +     (control (color-scheme-control cs))
>> > +     (sign-color (car colors))
>> > +     (attr-color (cadr colors))
>> > +     (str (color-scheme-str cs))
>> > +     (attrs (string->list 
>> > +             (call-with-input-string str (lambda (p) (read-delimited "(" 
>> > p))))))
>> > +      (call-with-output-string
>> > +       (lambda (port)
>> > +   (for-each (lambda (ch)
>> > +               (let ((color (if (is-sign? ch) sign-color attr-color)))
>> > +                 (display (color-it-inner color (string ch) control) 
>> > port)))
>> > +             attrs)
>> > +   (display (color-it-inner sign-color "(" control) port) ;; output 
>> > right-parent
>> > +   )))))
>> 
>> 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.

When we include features in Guile, we review the /implementation/ of
that feature in the hope that it’ll be reasonably pleasant our eyes.
This particular procedure could surely be made more pleasant to the eye. 
WDYT?

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

Using disjoint types is beneficial in helping catch programming errors,
and clarify what the objects being worked on are.

Again, this thing is part of the API, so it’s worth thinking it through.
Using a record makes it easier to eventually extend the thing.

So you may consider it necessary.

>> > +(define type-checker
>> > +  (lambda (data)
>> > +    (call/cc (lambda (return)
>> > +         (for-each (lambda (x)  ;; checkout user defined data type
>> > +                     (and ((car x) data) (return (cdr x))))
>> > +                   (current-custom-colorized))
>> > +         (for-each (lambda (x)  ;; checkout default data type
>> > +                     (and ((car x) data) (return (cdr x))))
>> > +                   *colorize-list*)
>> > +         (return `(UNKNOWN ,color-unknown (WHITE))))))) ;; no suitable 
>> > data type ,return the unknown solution
>> 
>> 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.

Sorry, it should be ‘find’, not ‘any’.

> It's here now:
> https://github.com/NalaGinrut/guile-colorized/blob/upstream/ice-9/colorized.scm

(Next time please post the code; this facilitates review.)

It seems it’s improved (thanks!), but I would like to see the API issues
and stylistic problems to be addressed.

> And I'm waiting for any help to write the test-case.

If you have specific questions as you work on it, I’m happy to help.
Otherwise, I won’t offer to write the actual tests.

BTW, before it can be integrated, it will also need to have a section in
the manual, probably under “Using Guile Interactively”.  Could you work
on it?

I reckon I’m asking for some extra work, but I think it’s important to
not compromise on Guile’s current standards.

Thank you!

Ludo’.