From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Nala Ginrut Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Colorized REPL Date: Tue, 22 Jan 2013 00:10:54 +0800 Message-ID: References: <1354692089.25329.71.camel@Renee-desktop.suse> <1354697316.25329.83.camel@Renee-desktop.suse> <1354703255.25329.107.camel@Renee-desktop.suse> <87a9tmajov.fsf@gnu.org> <1355106191.22533.63.camel@Renee-desktop.suse> <87zk1l1t3x.fsf@gnu.org> <1356942598.2785.174.camel@Renee-desktop.suse> <87623dvy88.fsf@gnu.org> <1357726654.2798.103.camel@Renee-desktop.suse> <87k3rj7psf.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary=047d7b6043c8ae06e304d3ceb422 X-Trace: ger.gmane.org 1358784698 6593 80.91.229.3 (21 Jan 2013 16:11:38 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 21 Jan 2013 16:11:38 +0000 (UTC) Cc: guile-devel To: =?UTF-8?Q?Ludovic_Court=C3=A8s?= Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Mon Jan 21 17:11:56 2013 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1TxJyQ-0003s5-DZ for guile-devel@m.gmane.org; Mon, 21 Jan 2013 17:11:50 +0100 Original-Received: from localhost ([::1]:34474 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TxJy9-00033J-AT for guile-devel@m.gmane.org; Mon, 21 Jan 2013 11:11:33 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:45137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TxJxe-0002PT-1M for guile-devel@gnu.org; Mon, 21 Jan 2013 11:11:06 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TxJxa-0000Te-U1 for guile-devel@gnu.org; Mon, 21 Jan 2013 11:11:01 -0500 Original-Received: from mail-ea0-f179.google.com ([209.85.215.179]:59410) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TxJxa-0000TO-GH; Mon, 21 Jan 2013 11:10:58 -0500 Original-Received: by mail-ea0-f179.google.com with SMTP id d12so1694912eaa.38 for ; Mon, 21 Jan 2013 08:10:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=kDX3MaHH0VAP8am0Ns1NxM+jbTmqVunPMgFV/gKZmls=; b=MijqEqq+FZcswxfRYB8Cu+DKFJU+ddOOgwx7H9XCU1uQ1tRdAJURwVf62t+P1k65ZN b7GAPrmLVDQLu401HfyCgmODdRjUBtTkMOz6k8H/sKR7b821CNocEHxwCaYGpp+7nOit InU/MLTlzYVlRdWk0yc6ITfNBigqw2fNOVyCREugr9X7TJf+38IJCrC1aJLPUCzTKVOe EurtzFpvpj9r2X+xz8HzyLLplxa6yEASg8seXIIipjSRnrj0VWHbNeiL8EFo5hDz58Nq BgHjFd9Kaw0QbfFSmHFVic9XrwVFK9qJeC+siChhGzDn3OvvoKTw2WrijFuLZ20flvAG jgrg== X-Received: by 10.14.3.137 with SMTP id 9mr50473863eeh.2.1358784654870; Mon, 21 Jan 2013 08:10:54 -0800 (PST) Original-Received: by 10.223.102.131 with HTTP; Mon, 21 Jan 2013 08:10:54 -0800 (PST) In-Reply-To: <87k3rj7psf.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 209.85.215.179 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:15492 Archived-At: --047d7b6043c8ae06e304d3ceb422 Content-Type: multipart/alternative; boundary=047d7b6043c8ae06e004d3ceb420 --047d7b6043c8ae06e004d3ceb420 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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=C3=A8s wrote= : > Hi Nala, > > Thanks for the update. > > Nala Ginrut skribis: > > > On Fri, 2013-01-04 at 15:06 +0100, Ludovic Court=C3=A8s wrote: > > [...] > > >> Nala Ginrut 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 i= n > >> 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 progra= m > >> > and output a colorful result for some monitoring purpose. > >> > PS: MY-LONG-NUM is an arbitrary name for your own color scheme, as y= ou > >> > like. > >> > >> Why is that name even needed? > > > > It's easy to debug or checkout the color-scheme info with the name. > > Hmm, there=E2=80=99s other info that helps debugging, such as location in= fo 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 ar= e > >> uncommon in Guile (like (define foo (lambda (arg) ...)), > >> *variable-with-stars*, hanging parentheses, etc.), sometimes weird > >> indentation, and use of tabs. > >> > >> Overall it=E2=80=99s essentially a new implementation of write/display= , so I=E2=80=99m 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=E2=80=99s a small set of constr= ucts to > express unit tests, such as =E2=80=98pass-if=E2=80=99. > > >> 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=E2=80=99s not the question. ;-) It doesn=E2=80=99t justify pulling = in all of R6RS. > > >> > +(define-record-type color-scheme > >> > + (fields str data type color control method)) > >> > >> Could you comment this? I=E2=80=99m 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 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=E2=80=99t 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=E2=80=99d expect (color-it str cs), but = instead > >> the string to be printed is embedded in the =E2=80=9Ccolor scheme=E2= =80=9D. > >> > > > > 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 =E2=80=98color-s= cheme=E2=80=99 > 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 colori= ng > >> > + ))) > >> > >> 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-delimite= d > "(" 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=E2=80=99ll be reasonably pleasant our ey= es. > 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 colo= r > >> setting? > >> > >> (define-record-type > >> (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=E2=80=99s worth thinking it t= hrough. > 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 =E2=80=98find=E2=80=99, not =E2=80=98any=E2=80=99. > > > It's here now: > > > https://github.com/NalaGinrut/guile-colorized/blob/upstream/ice-9/coloriz= ed.scm > > (Next time please post the code; this facilitates review.) > > It seems it=E2=80=99s 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=E2=80=99m happy to he= lp. > Otherwise, I won=E2=80=99t 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 =E2=80=9CUsing Guile Interactively=E2=80=9D. = Could you work > on it? > > I reckon I=E2=80=99m asking for some extra work, but I think it=E2=80=99s= important to > not compromise on Guile=E2=80=99s current standards. > > Thank you! > > Ludo=E2=80=99. > --047d7b6043c8ae06e004d3ceb420 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
hi folks!
test-case is ready. I've tested, it work= s 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, Ludovi= c Court=C3=A8s <ludo@gnu.org> wrote:
Hi Nala,

Thanks for the update.

Nala Ginrut <nalaginrut@gmail.co= m> skribis:

> On Fri, 2013-01-04 at 15:06 +0100, Ludovic Court=C3=A8s wrote:

[...]

>> Nala Ginrut <nalaginrut= @gmail.com> skribis:
>>
>> > 1. colorized-REPL feature:
>> > Add two lines to your ~/.guile, to enable colorized-REPL feat= ure:
>> > (use-modules (ice-9 colorized))
>> > (activate-colorized)
>>
>> I did that, and actually had to jump into a recursive REPL to see = it in
>> effect. =C2=A0Would 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)
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 (and (number? data) (> data 10000)))
>> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 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 schem= e, 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=E2=80=99s other info that helps debugging, such as locatio= n info of
the procedure, but OK.

>> Below is a rough review. =C2=A0There are many stylistic issues IMO= , such as
>> the lack of proper docstrings and comments, use of conventions tha= t are
>> uncommon in Guile (like (define foo (lambda (arg) ...)),
>> *variable-with-stars*, hanging parentheses, etc.), sometimes weird=
>> indentation, and use of tabs.
>>
>> Overall it=E2=80=99s essentially a new implementation of write/dis= play, so I=E2=80=99m a
>> bit concerned about keeping it in sync with the other one. =C2=A0C= ould 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. =C2=A0There=E2=80=99s a small set = of constructs to
express unit tests, such as =E2=80=98pass-if=E2=80=99.

>> Some other comments:
>>
>> > +(define-module (ice-9 colorized)
>> > + =C2=A0#:use-module (oop goops)
>> > + =C2=A0#:use-module ((rnrs) #:select (bytevector->u8-list= define-record-type
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vector-for-each bytevector?))
>>
>> Would be good to pull neither of these.
>>
>> Could you use (srfi srfi-9) and (rnrs bytevectors) instead of the<= br> >> latter? =C2=A0For GOOPS, see below.
>>
>
> record-type in r6rs is more convenient I think.

That=E2=80=99s not the question. ;-) =C2=A0It doesn=E2=80=99t justify= pulling in all of R6RS.

>> > +(define-record-type color-scheme
>> > + =C2=A0(fields str data type color control method))
>>
>> Could you comment this? =C2=A0I=E2=80=99m not clear on what each f= ield is.

Ping!

>> > +(define *color-list*
>> > + =C2=A0`((CLEAR =C2=A0 =C2=A0 =C2=A0 . =C2=A0 "0")=
>> > + =C2=A0 =C2=A0(RESET =C2=A0 =C2=A0 =C2=A0 . =C2=A0 "0&q= uot;)
>> > + =C2=A0 =C2=A0(BOLD =C2=A0 =C2=A0 =C2=A0 =C2=A0. =C2=A0 &quo= t;1")
>> > + =C2=A0 =C2=A0(DARK =C2=A0 =C2=A0 =C2=A0 =C2=A0. =C2=A0 &quo= t;2")
>> > + =C2=A0 =C2=A0(UNDERLINE =C2=A0 . =C2=A0 "4")
>> > + =C2=A0 =C2=A0(UNDERSCORE =C2=A0. =C2=A0 "4")
>> > + =C2=A0 =C2=A0(BLINK =C2=A0 =C2=A0 =C2=A0 . =C2=A0 "5&q= uot;)
>> > + =C2=A0 =C2=A0(REVERSE =C2=A0 =C2=A0 . =C2=A0 "6")=
>> > + =C2=A0 =C2=A0(CONCEALED =C2=A0 . =C2=A0 "8")
>> > + =C2=A0 =C2=A0(BLACK =C2=A0 =C2=A0 =C2=A0 . =C2=A0"30&q= uot;)
>> > + =C2=A0 =C2=A0(RED =C2=A0 =C2=A0 =C2=A0 =C2=A0 . =C2=A0"= ;31")
>> > + =C2=A0 =C2=A0(GREEN =C2=A0 =C2=A0 =C2=A0 . =C2=A0"32&q= uot;)
>> > + =C2=A0 =C2=A0(YELLOW =C2=A0 =C2=A0 =C2=A0. =C2=A0"33&q= uot;)
>> > + =C2=A0 =C2=A0(BLUE =C2=A0 =C2=A0 =C2=A0 =C2=A0. =C2=A0"= ;34")
>> > + =C2=A0 =C2=A0(MAGENTA =C2=A0 =C2=A0 . =C2=A0"35")=
>> > + =C2=A0 =C2=A0(CYAN =C2=A0 =C2=A0 =C2=A0 =C2=A0. =C2=A0"= ;36")
>> > + =C2=A0 =C2=A0(WHITE =C2=A0 =C2=A0 =C2=A0 . =C2=A0"37&q= uot;)
>> > + =C2=A0 =C2=A0(ON-BLACK =C2=A0 =C2=A0. =C2=A0"40")=
>> > + =C2=A0 =C2=A0(ON-RED =C2=A0 =C2=A0 =C2=A0. =C2=A0"41&q= uot;)
>> > + =C2=A0 =C2=A0(ON-GREEN =C2=A0 =C2=A0. =C2=A0"42")=
>> > + =C2=A0 =C2=A0(ON-YELLOW =C2=A0 . =C2=A0"43")
>> > + =C2=A0 =C2=A0(ON-BLUE =C2=A0 =C2=A0 . =C2=A0"44")=
>> > + =C2=A0 =C2=A0(ON-MAGENTA =C2=A0. =C2=A0"45")
>> > + =C2=A0 =C2=A0(ON-CYAN =C2=A0 =C2=A0 . =C2=A0"46")=
>> > + =C2=A0 =C2=A0(ON-WHITE =C2=A0 =C2=A0. =C2=A0"47")= ))
>>
>> Would it make sense to define a new type for colors? =C2=A0Like: >>
>> =C2=A0 (define-record-type <color>
>> =C2=A0 =C2=A0 (color foreground background attribute)
>> =C2=A0 =C2=A0 color?
>> =C2=A0 =C2=A0 ...)
>>
>> =C2=A0 (define light-cyan
>> =C2=A0 =C2=A0 (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? =C2=A0I still think that using a disjoint= type for
colors would be better than symbols. =C2=A0Also, this is something part of<= br> the API, so we can=E2=80=99t just leave it for later discussion.

>> > +(define color-it
>> > + =C2=A0(lambda (cs)
>> > + =C2=A0 =C2=A0(let* ((str (color-scheme-str cs))
>> > + =C2=A0 =C2=A0 (color (color-scheme-color cs))
>> > + =C2=A0 =C2=A0 (control (color-scheme-control cs)))
>> > + =C2=A0 =C2=A0 =C2=A0(color-it-inner color str control)))) >>
>> This is somewhat confusing: I=E2=80=99d expect (color-it str cs), = but instead
>> the string to be printed is embedded in the =E2=80=9Ccolor scheme= =E2=80=9D.
>>
>
> It's a convenient way to enclose string into 'color-scheme'= ;, since the
> string could be used later.

Surely, but it mixes concerns. =C2=A0Can you try to make sure =E2=80= =98color-scheme=E2=80=99
objects are just that, color scheme?

>> > +(define (backspace port)
>> > + =C2=A0(seek port -1 SEEK_CUR))
>>
>> What about non-seekable ports? =C2=A0Could it be avoided altogethe= r?
>>
>
> But I think the 'port' parameter in 'call-with-output-stri= ng' is always
> seekable, isn't it? The 'port' here is not a generic port.=

String ports are seekable, right. =C2=A0However, seeking here seems l= ike a
hack: you could just as well adjust the printer to not write that extra
character instead of writing it and then seeking backwards. =C2=A0WDYT?

>> > + =C2=A0 =C2=A0(if sign
>> > + =C2=A0(display (color-it-inner color sign control) port) = =C2=A0;; not array
>> > + =C2=A0(display (color-array-inner cs) port) ;; array comple= cated coloring
>> > + =C2=A0)))
>>
>> 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. =C2=A0Could you try to fix<= br> them as well?

>> > +(define color-array-inner
>> > + =C2=A0(lambda (cs)
>> > + =C2=A0 =C2=A0(let* ((colors (color-scheme-color cs))
>> > + =C2=A0 =C2=A0 (control (color-scheme-control cs))
>> > + =C2=A0 =C2=A0 (sign-color (car colors))
>> > + =C2=A0 =C2=A0 (attr-color (cadr colors))
>> > + =C2=A0 =C2=A0 (str (color-scheme-str cs))
>> > + =C2=A0 =C2=A0 (attrs (string->list
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (call-with-input-= string str (lambda (p) (read-delimited "(" p))))))
>> > + =C2=A0 =C2=A0 =C2=A0(call-with-output-string
>> > + =C2=A0 =C2=A0 =C2=A0 (lambda (port)
>> > + =C2=A0 (for-each (lambda (ch)
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (let ((col= or (if (is-sign? ch) sign-color attr-color)))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (di= splay (color-it-inner color (string ch) control) port)))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 attrs)
>> > + =C2=A0 (display (color-it-inner sign-color "(" co= ntrol) port) ;; output right-parent
>> > + =C2=A0 )))))
>>
>> Wow, this is hairy and heavyweight.
>>
>
> Yes, but the aim of colorized-REPL is to show more friendly UI to the<= br> > users, it dropped up some efficiency designs.

When we include features in Guile, we review the /implementation/ of<= br> that feature in the hope that it=E2=80=99ll be reasonably pleasant our eyes= .
This particular procedure could surely be made more pleasant to the eye. WDYT?

>> > +(define *colorize-list*
>> > + =C2=A0`((,integer? INTEGER ,color-integer (BLUE BOLD))
>> > + =C2=A0 =C2=A0(,char? CHAR ,color-char (YELLOW))
>>
>> Instead of a list, can you instead define a record for each token = color
>> setting?
>>
>> =C2=A0 (define-record-type <token-color>
>> =C2=A0 =C2=A0 (token-color name pred color-proc color)
>> =C2=A0 =C2=A0 token-color?
>> =C2=A0 =C2=A0 ...)
>>
>> =C2=A0 (define %token-colors
>> =C2=A0 =C2=A0 `(,(token-color 'integer integer? color-integer = '(blue bold))
>> =C2=A0 =C2=A0 =C2=A0 ...))
>>
>
> Hmm...if it's unnecessary, I prefer be lazy...

Using disjoint types is beneficial in helping catch programming error= s,
and clarify what the objects being worked on are.

Again, this thing is part of the API, so it=E2=80=99s worth thinking it thr= ough.
Using a record makes it easier to eventually extend the thing.

So you may consider it necessary.

>> > +(define type-checker
>> > + =C2=A0(lambda (data)
>> > + =C2=A0 =C2=A0(call/cc (lambda (return)
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 (for-each (lambda (x) =C2=A0;; = checkout user defined data type
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 (and ((car x) data) (return (cdr x))))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 (current-custom-colorized))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 (for-each (lambda (x) =C2=A0;; = checkout default data type
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 (and ((car x) data) (return (cdr x))))
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 *colorize-list*)
>> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 (return `(UNKNOWN ,color-unknow= n (WHITE))))))) ;; no suitable data type ,return the unknown solution
>>
>> Using call/cc here is fun but excessively bad-style. =C2=A0:-)
>>
>> Try something like:
>>
>> =C2=A0 (or (any ... (current-custom-colorized))
>> =C2=A0 =C2=A0 =C2=A0 (any ... %token-colors)
>> =C2=A0 =C2=A0 =C2=A0 (token-color 'unknown (const #t) color-un= known '(white)))
>>
>
> But in this context, I need a finder which could return the result, no= t
> just predicate true/false, 'any' seems can't provide that.=

Sorry, it should be =E2=80=98find=E2=80=99, not =E2=80=98any=E2=80=99= .

> 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=E2=80=99s improved (thanks!), but I would like to see the API i= ssues
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=E2=80=99m happy t= o help.
Otherwise, I won=E2=80=99t 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 =E2=80=9CUsing Guile Interactively=E2=80=9D. =C2= =A0Could you work
on it?

I reckon I=E2=80=99m asking for some extra work, but I think it=E2=80=99s i= mportant to
not compromise on Guile=E2=80=99s current standards.

Thank you!

Ludo=E2=80=99.

--047d7b6043c8ae06e004d3ceb420-- --047d7b6043c8ae06e304d3ceb422 Content-Type: application/octet-stream; name="colorized.test" Content-Disposition: attachment; filename="colorized.test" Content-Transfer-Encoding: base64 X-Attachment-Id: f_hc7t8grz0 Ozs7OyBjb2xvcml6ZWQudGVzdCAtLS0gdGVzdCAoaWNlLTkgY29sb3JpemVkKSBtb2R1bGUgLSot IHNjaGVtZSAtKi0KOzs7OyAKOzs7OyBDb3B5cmlnaHQgMjAxMiBGcmVlIFNvZnR3YXJlIEZvdW5k YXRpb24sIEluYy4KOzs7OyAKOzs7OyBUaGlzIGxpYnJhcnkgaXMgZnJlZSBzb2Z0d2FyZTsgeW91 IGNhbiByZWRpc3RyaWJ1dGUgaXQgYW5kL29yCjs7OzsgbW9kaWZ5IGl0IHVuZGVyIHRoZSB0ZXJt cyBvZiB0aGUgR05VIExlc3NlciBHZW5lcmFsIFB1YmxpYwo7Ozs7IExpY2Vuc2UgYXMgcHVibGlz aGVkIGJ5IHRoZSBGcmVlIFNvZnR3YXJlIEZvdW5kYXRpb247IGVpdGhlcgo7Ozs7IHZlcnNpb24g MyBvZiB0aGUgTGljZW5zZSwgb3IgKGF0IHlvdXIgb3B0aW9uKSBhbnkgbGF0ZXIgdmVyc2lvbi4K Ozs7OyAKOzs7OyBUaGlzIGxpYnJhcnkgaXMgZGlzdHJpYnV0ZWQgaW4gdGhlIGhvcGUgdGhhdCBp dCB3aWxsIGJlIHVzZWZ1bCwKOzs7OyBidXQgV0lUSE9VVCBBTlkgV0FSUkFOVFk7IHdpdGhvdXQg ZXZlbiB0aGUgaW1wbGllZCB3YXJyYW50eSBvZgo7Ozs7IE1FUkNIQU5UQUJJTElUWSBvciBGSVRO RVNTIEZPUiBBIFBBUlRJQ1VMQVIgUFVSUE9TRS4gIFNlZSB0aGUgR05VCjs7OzsgTGVzc2VyIEdl bmVyYWwgUHVibGljIExpY2Vuc2UgZm9yIG1vcmUgZGV0YWlscy4KOzs7OyAKOzs7OyBZb3Ugc2hv dWxkIGhhdmUgcmVjZWl2ZWQgYSBjb3B5IG9mIHRoZSBHTlUgTGVzc2VyIEdlbmVyYWwgUHVibGlj Cjs7OzsgTGljZW5zZSBhbG9uZyB3aXRoIHRoaXMgbGlicmFyeTsgaWYgbm90LCB3cml0ZSB0byB0 aGUgRnJlZSBTb2Z0d2FyZQo7Ozs7IEZvdW5kYXRpb24sIEluYy4sIDUxIEZyYW5rbGluIFN0cmVl dCwgRmlmdGggRmxvb3IsIEJvc3RvbiwgTUEgMDIxMTAtMTMwMSBVU0EKCihkZWZpbmUtbW9kdWxl ICh0ZXN0LXN1aXRlIHRlc3QtaWNlLTktY29sb3JpemVkKQogICM6dXNlLW1vZHVsZSAodGVzdC1z dWl0ZSBsaWIpCiAgIzp1c2UtbW9kdWxlIChvb3AgZ29vcHMpCiAgIzp1c2UtbW9kdWxlIChzcmZp IHNyZmktOSkKICAjOnVzZS1tb2R1bGUgKGljZS05IGNvbG9yaXplZCkpCgo7OyBlbmFibGUgY29s b3JpemVkLVJFUEwgdGVzdCBwcmludGVyCigoQEAgKGljZS05IGNvbG9yaXplZCkgZW5hYmxlLWNv bG9yLXRlc3QpKQoKOzs7Cjs7OyBjb2xvcnplZCBvYmplY3QgdGVzdAo7OzsKCihkZWZpbmUgKHRl c3QtbWUgb2JqKQogIChwYXNzLWlmICJPSyIgCiAgICAgICAgICAgKGVxdWFsPyAoY2FsbC13aXRo LW91dHB1dC1zdHJpbmcKICAgICAgICAgICAgICAgICAgICAobGFtYmRhIChwb3J0KSAoY29sb3Jp emUgb2JqIHBvcnQpKSkKICAgICAgICAgICAgICAgICAgIChvYmplY3QtPnN0cmluZyBvYmopKSkp Cgood2l0aC10ZXN0LXByZWZpeCAiY29sb3JpemVkIG9iamVjdCB0ZXN0cyIKCiAgKHdpdGgtdGVz dC1wcmVmaXggImludGVnZXIiCiAgICAodGVzdC1tZSAxMjMpKQoKICAod2l0aC10ZXN0LXByZWZp eCAiY2hhciIKICAgICh0ZXN0LW1lICNcYykpCgogICh3aXRoLXRlc3QtcHJlZml4ICJzdHJpbmci CiAgICAodGVzdC1tZSAiaGVsbG8gd29ybGRcbiIpKQoKICAod2l0aC10ZXN0LXByZWZpeCAibGlz dCIKICAgICh0ZXN0LW1lICcoMSAyIDMgNCA1KSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJwYWly IgogICAgKHRlc3QtbWUgKGNvbnMgMSAyKSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJjbGFzcyIK ICAgICh0ZXN0LW1lIDxpbnRlZ2VyPikpCgogICh3aXRoLXRlc3QtcHJlZml4ICJwcm9jZWR1cmUi CiAgICAodGVzdC1tZSArKSkKCiAgKHdpdGgtdGVzdC1wcmVmaXggInZlY3RvciIKICAgICh0ZXN0 LW1lICh2ZWN0b3IgMSAyIDMpKSkKCiAgKHdpdGgtdGVzdC1wcmVmaXggImtleXdvcmQiCiAgICAo dGVzdC1tZSAjOnRlc3QtbWUpKQoKICAod2l0aC10ZXN0LXByZWZpeCAiY2hhci1zZXQiCiAgICAo dGVzdC1tZSBjaGFyLXNldDphc2NpaSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJzeW1ib2wiCiAg ICAodGVzdC1tZSAndGVzdC1tZSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJzdGFjayIKICAgICh0 ZXN0LW1lIChtYWtlLXN0YWNrICN0KSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJyZWNvcmQtdHlw ZSIKICAgIChkZWZpbmUtcmVjb3JkLXR5cGUgYWFhIChtYWtlLWFhYSBhKSBhYWE/IChhIGEpKQog ICAgKHRlc3QtbWUgYWFhKSkKCiAgKHdpdGgtdGVzdC1wcmVmaXggImluZXhhY3QiCiAgICAodGVz dC1tZSAxLjIpKQoKICAod2l0aC10ZXN0LXByZWZpeCAiZXhhY3QiCiAgICAodGVzdC1tZSAxLzIp KQoKICAod2l0aC10ZXN0LXByZWZpeCAicmVnZXhwIgogICAgKHRlc3QtbWUgKG1ha2UtcmVnZXhw ICJbMC05XSoiKSkpCgogICh3aXRoLXRlc3QtcHJlZml4ICJiaXR2ZWN0b3IiCiAgICAodGVzdC1t ZSAobWFrZS1iaXR2ZWN0b3IgOCkpKQoKICAod2l0aC10ZXN0LXByZWZpeCAiYXJyYXkiCiAgICAo dGVzdC1tZSAjMnUzMkAyQDMoKDEgMikgKDMgNCkpKSkKCiAgKHdpdGgtdGVzdC1wcmVmaXggImJv b2xlYW4iCiAgICAodGVzdC1tZSAjZikKICAgICh0ZXN0LW1lICN0KSkKCiAgKHdpdGgtdGVzdC1w cmVmaXggImNvbXBsZXgiCiAgICAodGVzdC1tZSAzKzRpKSkKCiAgKHdpdGgtdGVzdC1wcmVmaXgg Imhhc2ggdGFibGUiCiAgICAodGVzdC1tZSAobWFrZS1oYXNoLXRhYmxlKSkpCgogICh3aXRoLXRl c3QtcHJlZml4ICJob29rIgogICAgKHRlc3QtbWUgKG1ha2UtaG9vaykpKSkKCiAgCg== --047d7b6043c8ae06e304d3ceb422--