Re: [PATCH] Colorized REPL

[Nala Ginrut]

On Thu, 2013-01-10 at 16:19 +0800, Daniel Hartwig wrote:
> 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.
> 

Well, as I mentioned before, not all people use Emacs, and many of them,
especially newbies, they may never tried Emacs/vi.

> 
> > string-in-color
> 
> colorize-string is a much nicer name.
> 

Agreed~
I changed these:
string-in-color => colorize-string
display-string-in-color => colorized-display

What do you think?

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

Alright, fixed.

> 
> 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?!
> 

It's more conspicuous for the users. I asked some guys, they like it.

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

But how to check if a REPL is already running?

> >> 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”.
> 

Colorize an 'obj' is strange, I think 'data' is better.

> >>
> >> 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)
> 

It's an important correction, I realized that I don't have to handle
bytevector, it's an special array too, and I don't have to import (rnrs
bytevectors), thanks for pointing out it!
I believe the bugs above all fixed now. 
But what's the expected result of "(write #u8(0 1 2)) (newline)"? 

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

> >> 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:
> 

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.

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

Anyway, I'll keep it.

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

OK, I think it's an absolute design, fixed. 

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

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~

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

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

I removed bytevectors since it's unnecessary.

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

Right~it's a nice thing but I misunderstood it. Fixed.

> Best wishes.

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

It become better and better now, no matter if it's applied, it's a nice
thing to play, and I learned many things from this work. Thanks guys!