From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Colorized REPL Date: Fri, 11 Jan 2013 15:33:20 +0100 Message-ID: <87k3rj7psf.fsf@gnu.org> 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> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1357914823 2354 80.91.229.3 (11 Jan 2013 14:33:43 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 11 Jan 2013 14:33:43 +0000 (UTC) Cc: guile-devel@gnu.org To: Nala Ginrut Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Jan 11 15:33:59 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 1TtfgD-0000Z3-VM for guile-devel@m.gmane.org; Fri, 11 Jan 2013 15:33:58 +0100 Original-Received: from localhost ([::1]:59333 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ttffw-0004Sk-IX for guile-devel@m.gmane.org; Fri, 11 Jan 2013 09:33:40 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:53740) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ttffl-0004SQ-PL for guile-devel@gnu.org; Fri, 11 Jan 2013 09:33:38 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ttffh-0000xV-AM for guile-devel@gnu.org; Fri, 11 Jan 2013 09:33:29 -0500 Original-Received: from mail4-relais-sop.national.inria.fr ([192.134.164.105]:26736) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ttffg-0000xP-Sg for guile-devel@gnu.org; Fri, 11 Jan 2013 09:33:25 -0500 X-IronPort-AV: E=Sophos;i="4.84,451,1355094000"; d="scan'208";a="168159751" Original-Received: from reverse-83.fdn.fr (HELO pluto) ([80.67.176.83]) by mail4-relais-sop.national.inria.fr with ESMTP/TLS/DHE-RSA-AES128-SHA; 11 Jan 2013 15:33:21 +0100 X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 22 =?utf-8?Q?Niv=C3=B4se?= an 221 de la =?utf-8?Q?R?= =?utf-8?Q?=C3=A9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 83C4 F8E5 10A3 3B4C 5BEA D15D 77DD 95E2 EA52 ECF4 X-OS: x86_64-unknown-linux-gnu In-Reply-To: <1357726654.2798.103.camel@Renee-desktop.suse> (Nala Ginrut's message of "Wed, 09 Jan 2013 18:17:34 +0800") User-Agent: Gnus/5.130005 (Ma Gnus v0.5) Emacs/24.2 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 192.134.164.105 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:15383 Archived-At: 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: >>=20 >> > 1. colorized-REPL feature: >> > Add two lines to your ~/.guile, to enable colorized-REPL feature:=20 >> > (use-modules (ice-9 colorized)) >> > (activate-colorized) >>=20 >> I did that, and actually had to jump into a recursive REPL to see it in >> effect. Would be nice to fix it. >>=20 > > Well, I'm not sure what's the mean of 'recursive REPL'?=20 An inner REPL (info "(guile) Error Handling"). >> > 2. custom color scheme: >> > Example: >> > (add-color-scheme! `((,(lambda (data)=20 >> > (and (number? data) (> data 10000))) >> > MY-LONG-NUM ,color-it (RED)))) >>=20 >> Nice. >>=20 >> > 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. >>=20 >> 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 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. >>=20 >> 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? >>=20 > > 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 construc= ts to express unit tests, such as =E2=80=98pass-if=E2=80=99. >> Some other comments: >>=20 >> > +(define-module (ice-9 colorized) >> > + #:use-module (oop goops) >> > + #:use-module ((rnrs) #:select (bytevector->u8-list define-record-ty= pe >> > + vector-for-each bytevector?)) >>=20 >> Would be good to pull neither of these. >>=20 >> Could you use (srfi srfi-9) and (rnrs bytevectors) instead of the >> latter? For GOOPS, see below. >>=20 > > 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)) >>=20 >> 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"))) >>=20 >> Would it make sense to define a new type for colors? Like: >>=20 >> (define-record-type >> (color foreground background attribute) >> color? >> ...) >>=20 >> (define light-cyan >> (color x y z)) >>=20 > > 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)))) >>=20 >> This is somewhat confusing: I=E2=80=99d expect (color-it str cs), but in= stead >> the string to be printed is embedded in the =E2=80=9Ccolor scheme=E2=80= =9D. >>=20 > > It's a convenient way to enclose string into 'color-scheme', since the > string could be used later.=20 Surely, but it mixes concerns. Can you try to make sure =E2=80=98color-sch= eme=E2=80=99 objects are just that, color scheme? >> > +(define (backspace port) >> > + (seek port -1 SEEK_CUR)) >>=20 >> What about non-seekable ports? Could it be avoided altogether? >>=20 > > 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 >> > + ))) >>=20 >> Parentheses should be at the end of the previous line. >> End-of-line comments should be introduced with a single semicolon. >>=20 > > 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=20 >> > + (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 ri= ght-parent >> > + ))))) >>=20 >> Wow, this is hairy and heavyweight. >>=20 > > 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 eyes. This particular procedure could surely be made more pleasant to the eye.=20 WDYT? >> > +(define *colorize-list* >> > + `((,integer? INTEGER ,color-integer (BLUE BOLD)) >> > + (,char? CHAR ,color-char (YELLOW)) >>=20 >> Instead of a list, can you instead define a record for each token color >> setting? >>=20 >> (define-record-type >> (token-color name pred color-proc color) >> token-color? >> ...) >>=20 >> (define %token-colors >> `(,(token-color 'integer integer? color-integer '(blue bold)) >> ...)) >>=20 > > 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 thr= ough. 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 >>=20 >> Using call/cc here is fun but excessively bad-style. :-) >>=20 >> Try something like: >>=20 >> (or (any ... (current-custom-colorized)) >> (any ... %token-colors) >> (token-color 'unknown (const #t) color-unknown '(white))) >>=20 > > 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 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 to 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. Co= uld 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.