Re: [PATCH] Colorized REPL

[Nala Ginrut]

On Sun, 2013-01-27 at 11:06 +0100, Andy Wingo wrote:
> On Sat 26 Jan 2013 11:15, Nala Ginrut <address@hidden> writes:
> 
> > Please review it. ;-P
> 
> A drive-by review (i.e., just style comments and random questions)
> 
> > ;; Copyright (C) 2012 Free Software Foundation, Inc.
> 
> 2013
> 
> > ;;;; Author: Mu Lei known as NalaGinrut <address@hidden>
> 
> > (define-module (ice-9 colorized)
> >   #:use-module (ice-9 rdelim)
> >   #:use-module ((srfi srfi-1) #:select (filter-map any proper-list?))
> >   #:use-module (srfi srfi-9)
> >   #:use-module (system repl common)
> >   #:export (activate-colorized custom-colorized-set! color-it colorize-it 
> > colorize
> >         colorize-string colorized-display add-color-scheme!))
> 
> No tabs, please.  In emacs this is (indent-tabs-mode nil) I think; you
> can M-x untabify also.
> 

Sorry, I thought I did...

> > (define (activate-colorized)
> >   (let ((rs (fluid-ref *repl-stack*)))
> >     (if (null? rs)
> >     (repl-default-option-set! 'print colorized-repl-printer) ; if no REPL 
> > started, set as default printer
> >     (repl-option-set! (car rs) 'print colorized-repl-printer)))) ; or set 
> > as the top-REPL printer
> 
> Nice
> 
> > (define *color-list*
> >   `((CLEAR       .   "0")
> 
> Why are these upper-cased?  Unless there is a reason, lower-case is
> better as it is easier to type and read.
> 

Well, this issue was discussed several times in this thread.
Originally I used symbol with lowercase for that, and Daniel suggested
me merge (term ansi-color) in guile-lib which is has this style. Then we
found that there're less things could be borrowed from (term
ansi-color), but I was suggested keep this style for some reasons.
Anyway, I don't think this style matters. ;-)

> > (define (get-color color)
> >   (assoc-ref *color-list* color))
> 
> Use assq-ref, the keys are symbols
> 
> > (define (generate-color colors)
> >   (let ((color-list
> >      (filter-map (lambda (c) (assoc-ref *color-list* c)) colors)))
> 
> Use get-color here; 

hmm...neglected... 

> and is the intention to silently ignore unknown
> colors?
> 

Yes, I think so. It won't break any think, except for redundant
ansi-control string appended.
And for undefined color-scheme for certain object, there's an 'unknown'
color-scheme for that.

> > (define (colorize-the-string color str control)
> >   (string-append (generate-color color) str (generate-color control)))
> 
> The name is confusingly like "colorize-string" below.  Better to have a
> more descriptive name, or maybe colorize-string-helper.
> 

Fixed.

> > ;; test-helper functions
> > ;; when eanbled, it won't output colored result, but just normal.
> > ;; it used to test the array/list/vector print result.
> > (define *color-func* (make-fluid colorize-the-string))
> > (define (disable-color-test) 
> >   (fluid-set! *color-func* colorize-the-string))
> > (define (enable-color-test) 
> >   (fluid-set! *color-func* color-it-test))
> 
> Surely testing-related functions should not be here?
> 

Daniel suggest me strip all the escape sequence from the result.
But I have some trouble with the regexp in Guile, seems "\\d+" can't
work, others, like "\\w+" works.
Maybe I should start a new thread about more details usage about regexp
in Guile? 

> > (define (color-it-inner color str control)
> >   ((fluid-ref *color-func*) color str control))
> 
> Use parameters instead of fluids.
> 
> > (define *pre-sign* 
> >   `((LIST       .   "(") 
> >     (PAIR       .   "(") 
> >     (VECTOR     .   "#(")
> >     (ARRAY      .   #f))) 
> > ;; array's sign is complecated, return #f so it will be handled by pre-print
> 
> "complicated"
> 

Shamed... :-(

> > (define* (pre-print cs #:optional (port (current-output-port)))
> >   (let* ((type (color-scheme-type cs))
> >      (control (color-scheme-control cs))
> >      (sign (assoc-ref *pre-sign* type))
> 
> assq-ref

Fixed.

> 
> > (define *colorize-list*
> >   `((,integer? INTEGER ,color-integer (BLUE BOLD))
> >     (,char? CHAR ,color-char (YELLOW))
> >     (,string? STRING ,color-string (RED))
> 
> Interesting :)
> 
> Regards,
> 
> Andy