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, 04 Jan 2013 15:06:47 +0100 Message-ID: <87623dvy88.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> 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 1357308416 30183 80.91.229.3 (4 Jan 2013 14:06:56 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 4 Jan 2013 14:06:56 +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 04 15:07:13 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 1Tr7vU-0003a0-DC for guile-devel@m.gmane.org; Fri, 04 Jan 2013 15:07:12 +0100 Original-Received: from localhost ([::1]:43612 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tr7vF-0008Dt-2U for guile-devel@m.gmane.org; Fri, 04 Jan 2013 09:06:57 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:42840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tr7vA-0008DH-TD for guile-devel@gnu.org; Fri, 04 Jan 2013 09:06:55 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tr7v8-00038v-TH for guile-devel@gnu.org; Fri, 04 Jan 2013 09:06:52 -0500 Original-Received: from mail1-relais-roc.national.inria.fr ([192.134.164.82]:52479) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tr7v8-00036V-FD for guile-devel@gnu.org; Fri, 04 Jan 2013 09:06:50 -0500 X-IronPort-AV: E=Sophos;i="4.84,409,1355094000"; d="scan'208";a="188471007" Original-Received: from reverse-83.fdn.fr (HELO pluto) ([80.67.176.83]) by mail1-relais-roc.national.inria.fr with ESMTP/TLS/DHE-RSA-AES128-SHA; 04 Jan 2013 15:06:47 +0100 X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 15 =?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: <1356942598.2785.174.camel@Renee-desktop.suse> (Nala Ginrut's message of "Mon, 31 Dec 2012 16:29:58 +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.82 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:15368 Archived-At: Hi Nala, Thanks for your work! Nala Ginrut skribis: > 1. colorized-REPL feature: > Add two lines to your ~/.guile, to enable colorized-REPL feature:=20 > (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. Once in effect, the result is pleasant. :-) > 2. custom color scheme: > Example: > (add-color-scheme! `((,(lambda (data)=20 > (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? 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=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? 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. > +(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. > +(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)) > +(define generate-color > + (lambda (colors) > + (let ((color-list=20 > + (remove not=20 > + (map (lambda (c) (assoc-ref *color-list* c)) colors)))) Use filter-map instead. > +(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 inste= ad the string to be printed is embedded in the =E2=80=9Ccolor scheme=E2=80=9D. > +(define (backspace port) > + (seek port -1 SEEK_CUR)) What about non-seekable ports? Could it be avoided altogether? > +(define *pre-sign*=20 > + `((LIST . "(")=20 > + (PAIR . "(")=20 > + (VECTOR . "#(") > + (BYTEVECTOR . "#vu8(") > + (ARRAY . #f))) ;; array's sign is complecated. It=E2=80=99s complicated, so what? :-) The comment should instead mention that arrays get special treatment in =E2=80=98pre-print=E2=80=99. > +(define* (pre-print cs #:optional (port (current-output-port))) > + (let* ((type (color-scheme-type cs)) > + (control (color-scheme-control cs)) > + (sign (assoc-ref *pre-sign* type)) > + (color (color-scheme-color cs))) ;; (car color) is the color, (cdr col= or) is the control Is that comment necessary here? > + (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. > +(define is-sign? > + (lambda (ch) > + (char-set-contains? char-set:punctuation ch))) Perhaps =E2=80=98delimiter?=E2=80=99 would be a better name? > +(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 right= -parent > + ))))) Wow, this is hairy and heavyweight. > +;; I believe all end-sign is ")"=20=20=20=20=20=20 > +(define* (post-print cs #:optional (port (current-output-port))) > + (let* ((c (color-scheme-color cs)) > + (control (color-scheme-control cs)) > + (color (if (list? (car c)) (car c) c))) ;; array has a color-list > + (display (color-it-inner color ")" control) port))) Instead of the comment above, add a docstring that says =E2=80=9CWrite a cl= osing parenthesis...=E2=80=9D. > +(define *custom-colorized-list* (make-fluid '())) It=E2=80=99s better to use SRFI-39 parameters (which are in core now). > +(define (class? obj) > + (is-a? obj )) It=E2=80=99s enough to use =E2=80=98struct?=E2=80=99 since objects are stru= cts. This way you get rid of the dependency on GOOPS. > +(define (arbiter? obj) > + (is-a? obj )) Who care about arbiters? :-) > +(define (unknown? obj) > + (is-a? obj )) This one isn=E2=80=99t needed: it=E2=80=99s just the =E2=80=98else=E2=80=99= case. > +(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 name pred color-proc color) token-color? ...) (define %token-colors `(,(token-color 'integer integer? color-integer '(blue bold)) ...)) > +(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 da= ta 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))) Also, the name is misleading. Should be called =E2=80=98data->token-color= =E2=80=99 or something like that. > +(define string-in-color > + (lambda (str color) > +"@code{string-in-color}. The argument @var{color} is the color list. > + Example: (string-in-color \"hello\" '(BLUE BOLD))"=20 No Texinfo escapes in docstrings. Thanks, Ludo=E2=80=99.