hi folks! test-case is ready. I've tested, it works and passed all items. Please review it. I'll work on manual for it ASAP.
Thanks!
On Fri, Jan 11, 2013 at 10:33 PM, Ludovic Courtès <l...@gnu.org> wrote:
> Hi Nala,
>
> Thanks for the update.
>
> Nala Ginrut <nalagin...@gmail.com> skribis:
>
> > On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
>
> [...]
>
> >> 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'?
>
> An inner REPL (info "(guile) Error Handling").
>
> >> > 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.
>
> Hmm, there’s other info that helps debugging, such as location info of
> the procedure, but OK.
>
> >> 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?
>
> See under test-suite/tests/*.test. There’s a small set of constructs to
> express unit tests, such as ‘pass-if’.
>
> >> 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.
>
> That’s not the question. ;-) It doesn’t justify pulling in all of R6RS.
>
> >> > +(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.
>
> Ping!
>
> >> > +(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...
>
> Which implementation? I still think that using a disjoint type for
> colors would be better than symbols. Also, this is something part of
> the API, so we can’t just leave it for later discussion.
>
> >> > +(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.
>
> Surely, but it mixes concerns. Can you try to make sure ‘color-scheme’
> objects are just that, color scheme?
>
> >> > +(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.
>
> String ports are seekable, right. However, seeking here seems like a
> hack: you could just as well adjust the printer to not write that extra
> character instead of writing it and then seeking backwards. WDYT?
>
> >> > + (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. ;-)
>
> There are still many conventions wrong, such as procedure definitions,
> global variable names, missing docstrings, etc. Could you try to fix
> them as well?
>
> >> > +(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.
>
> When we include features in Guile, we review the /implementation/ of
> 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?
>
> >> > +(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...
>
> Using disjoint types is beneficial in helping catch programming errors,
> 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.
>
> >> > +(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.
>
> Sorry, it should be ‘find’, not ‘any’.
>
> > It's here now:
> >
> https://github.com/NalaGinrut/guile-colorized/blob/upstream/ice-9/colorized.scm
>
> (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.
>
> > And I'm waiting for any help to write the test-case.
>
> If you have specific questions as you work on it, I’m happy to help.
> 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
