On Sun, 2013-01-27 at 11:06 +0100, Andy Wingo wrote:
> On Sat 26 Jan 2013 11:15, Nala Ginrut <nalagin...@gmail.com> 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 <nalagin...@gmail.com>
>
> > (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
