Re: [PATCH] Colorized REPL

[Nala Ginrut <nalaginrut@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10788 bytes --]

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 <ludo@gnu.org> wrote:

> Hi Nala,
>
> Thanks for the update.
>
> Nala Ginrut <nalaginrut@gmail.com> skribis:
>
> > On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
>
> [...]
>
> >> Nala Ginrut <nalaginrut@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’.
>

[-- Attachment #1.2: Type: text/html, Size: 14414 bytes --]

[-- Attachment #2: colorized.test --]
[-- Type: application/octet-stream, Size: 2728 bytes --]

;;;; colorized.test --- test (ice-9 colorized) module -*- scheme -*-
;;;; 
;;;; Copyright 2012 Free Software Foundation, Inc.
;;;; 
;;;; This library is free software; you can redistribute it and/or
;;;; modify it under the terms of the GNU Lesser General Public
;;;; License as published by the Free Software Foundation; either
;;;; version 3 of the License, or (at your option) any later version.
;;;; 
;;;; This library is distributed in the hope that it will be useful,
;;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
;;;; Lesser General Public License for more details.
;;;; 
;;;; You should have received a copy of the GNU Lesser General Public
;;;; License along with this library; if not, write to the Free Software
;;;; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

(define-module (test-suite test-ice-9-colorized)
  #:use-module (test-suite lib)
  #:use-module (oop goops)
  #:use-module (srfi srfi-9)
  #:use-module (ice-9 colorized))

;; enable colorized-REPL test printer
((@@ (ice-9 colorized) enable-color-test))

;;;
;;; colorzed object test
;;;

(define (test-me obj)
  (pass-if "OK" 
           (equal? (call-with-output-string
                    (lambda (port) (colorize obj port)))
                   (object->string obj))))

(with-test-prefix "colorized object tests"

  (with-test-prefix "integer"
    (test-me 123))

  (with-test-prefix "char"
    (test-me #\c))

  (with-test-prefix "string"
    (test-me "hello world\n"))

  (with-test-prefix "list"
    (test-me '(1 2 3 4 5)))

  (with-test-prefix "pair"
    (test-me (cons 1 2)))

  (with-test-prefix "class"
    (test-me <integer>))

  (with-test-prefix "procedure"
    (test-me +))

  (with-test-prefix "vector"
    (test-me (vector 1 2 3)))

  (with-test-prefix "keyword"
    (test-me #:test-me))

  (with-test-prefix "char-set"
    (test-me char-set:ascii))

  (with-test-prefix "symbol"
    (test-me 'test-me))

  (with-test-prefix "stack"
    (test-me (make-stack #t)))

  (with-test-prefix "record-type"
    (define-record-type aaa (make-aaa a) aaa? (a a))
    (test-me aaa))

  (with-test-prefix "inexact"
    (test-me 1.2))

  (with-test-prefix "exact"
    (test-me 1/2))

  (with-test-prefix "regexp"
    (test-me (make-regexp "[0-9]*")))

  (with-test-prefix "bitvector"
    (test-me (make-bitvector 8)))

  (with-test-prefix "array"
    (test-me #2u32@2@3((1 2) (3 4))))

  (with-test-prefix "boolean"
    (test-me #f)
    (test-me #t))

  (with-test-prefix "complex"
    (test-me 3+4i))

  (with-test-prefix "hash table"
    (test-me (make-hash-table)))

  (with-test-prefix "hook"
    (test-me (make-hook))))