|
From: | Nala Ginrut |
Subject: | Re: [PATCH] Colorized REPL |
Date: | Tue, 22 Jan 2013 00:10:54 +0800 |
Hi Nala,
Thanks for the update.
Nala Ginrut <address@hidden> skribis:
[...]
> On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
An inner REPL (info "(guile) Error Handling").
>> Nala Ginrut <address@hidden> 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'?
Hmm, there’s other info that helps debugging, such as location info of
>> > 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.
the procedure, but OK.
See under test-suite/tests/*.test. There’s a small set of constructs to
>> 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?
express unit tests, such as ‘pass-if’.
That’s not the question. ;-) It doesn’t justify pulling in all of R6RS.
>> 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.
Ping!
>> > +(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.
Which implementation? I still think that using a disjoint type for
>> > +(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...
colors would be better than symbols. Also, this is something part of
the API, so we can’t just leave it for later discussion.
Surely, but it mixes concerns. Can you try to make sure ‘color-scheme’
>> > +(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.
objects are just that, color scheme?
String ports are seekable, right. However, seeking here seems like a
>> > +(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.
hack: you could just as well adjust the printer to not write that extra
character instead of writing it and then seeking backwards. WDYT?
There are still many conventions wrong, such as procedure definitions,
>> > + (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. ;-)
global variable names, missing docstrings, etc. Could you try to fix
them as well?
When we include features in Guile, we review the /implementation/ of
>> > +(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.
that feature in the hope that it’ll be reasonably pleasant our eyes.
This particular procedure could surely be made more pleasant to the eye.
WDYT?
Using disjoint types is beneficial in helping catch programming errors,
>> > +(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...
and clarify what the objects being worked on are.
Again, this thing is part of the API, so it’s worth thinking it through.
Using a record makes it easier to eventually extend the thing.
So you may consider it necessary.
Sorry, it should be ‘find’, not ‘any’.
>> > +(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.
(Next time please post the code; this facilitates review.)
It seems it’s improved (thanks!), but I would like to see the API issues
and stylistic problems to be addressed.
If you have specific questions as you work on it, I’m happy to help.
> And I'm waiting for any help to write the test-case.
Otherwise, I won’t offer to write the actual tests.
BTW, before it can be integrated, it will also need to have a section in
the manual, probably under “Using Guile Interactively”. Could you work
on it?
I reckon I’m asking for some extra work, but I think it’s important to
not compromise on Guile’s current standards.
Thank you!
Ludo’.
colorized.test
Description: Binary data
[Prev in Thread] | Current Thread | [Next in Thread] |