On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
> Hi Nala,
>
> Thanks for your work!
>
> Nala Ginrut <nalagin...@gmail.com> 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'?
> Once in effect, the result is pleasant. :-)
>
I'm glad you like it. ;-D
> > 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.
>
> 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?
> 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.
> > +(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.
>
> > +(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...
> > +(define generate-color
> > + (lambda (colors)
> > + (let ((color-list
> > + (remove not
> > + (map (lambda (c) (assoc-ref *color-list* c)) colors))))
>
> Use filter-map instead.
>
nice to know that~
> > +(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.
> > +(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.
> > +(define *pre-sign*
> > + `((LIST . "(")
> > + (PAIR . "(")
> > + (VECTOR . "#(")
> > + (BYTEVECTOR . "#vu8(")
> > + (ARRAY . #f))) ;; array's sign is complecated.
>
> It’s complicated, so what? :-)
>
> The comment should instead mention that arrays get special treatment in
> ‘pre-print’.
>
> > +(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))
> > + (color (color-scheme-color cs))) ;; (car color) is the color, (cdr
> > color) is the control
>
> Is that comment necessary here?
Ah~thanks for pointing out, it's the obsolete design.
> > + (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. ;-)
> > +(define is-sign?
> > + (lambda (ch)
> > + (char-set-contains? char-set:punctuation ch)))
>
> Perhaps ‘delimiter?’ would be a better name?
>
Agreed~
> > +(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.
I do considered a more efficient way to print simpler colorized-array,
but I decide to print it like this finally. I believe a more clear
array-print-result make users hate arrays less, since it's too
complicated in Guile, though we don't have to use it in complicated way
all the time.
> > +;; I believe all end-sign is ")"
> > +(define* (post-print cs #:optional (port (current-output-port)))
> > + (let* ((c (color-scheme-color cs))
> > + (control (color-scheme-control cs))
> > + (color (if (list? (car c)) (car c) c))) ;; array has a color-list
> > + (display (color-it-inner color ")" control) port)))
>
> Instead of the comment above, add a docstring that says “Write a closing
> parenthesis...”.
>
> > +(define *custom-colorized-list* (make-fluid '()))
>
> It’s better to use SRFI-39 parameters (which are in core now).
>
Well, fluid is easier. ;-P
> > +(define (class? obj)
> > + (is-a? obj <class>))
>
> It’s enough to use ‘struct?’ since objects are structs. This way you
> get rid of the dependency on GOOPS.
>
> > +(define (arbiter? obj)
> > + (is-a? obj <arbiter>))
>
> Who care about arbiters? :-)
>
> > +(define (unknown? obj)
> > + (is-a? obj <unknown>))
>
> This one isn’t needed: it’s just the ‘else’ case.
>
Agreed~
> > +(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
> > + (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.
Any other suggestion?
> Also, the name is misleading. Should be called ‘data->token-color’ or
> something like that.
>
Agreed~
> > +(define string-in-color
> > + (lambda (str color)
> > +"@code{string-in-color}. The argument @var{color} is the color list.
> > + Example: (string-in-color \"hello\" '(BLUE BOLD))"
>
> No Texinfo escapes in docstrings.
>
Agreed~
> Thanks,
> Ludo’.
It's here now:
https://github.com/NalaGinrut/guile-colorized/blob/upstream/ice-9/colorized.scm
And I'm waiting for any help to write the test-case.
Thanks!
