Re: [PATCH] Colorized REPL

[Andy Wingo]

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.

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

> (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; and is the intention to silently ignore unknown
colors?

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

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

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

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

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

Interesting :)

Regards,

Andy
-- 
http://wingolog.org/