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: Wed, 09 Jan 2013 18:17:34 +0800 Organization: HFG Message-ID: <1357726654.2798.103.camel@Renee-desktop.suse> 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> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Trace: ger.gmane.org 1357726671 21188 80.91.229.3 (9 Jan 2013 10:17:51 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 9 Jan 2013 10:17:51 +0000 (UTC) Cc: guile-devel@gnu.org To: Ludovic =?ISO-8859-1?Q?Court=E8s?= Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Wed Jan 09 11:18:09 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 1TssjY-0000Fp-NR for guile-devel@m.gmane.org; Wed, 09 Jan 2013 11:18:08 +0100 Original-Received: from localhost ([::1]:60579 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TssjI-0002iK-W3 for guile-devel@m.gmane.org; Wed, 09 Jan 2013 05:17:52 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:55210) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TssjE-0002ag-4j for guile-devel@gnu.org; Wed, 09 Jan 2013 05:17:50 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TssjB-0001s4-EV for guile-devel@gnu.org; Wed, 09 Jan 2013 05:17:48 -0500 Original-Received: from mail-da0-f52.google.com ([209.85.210.52]:56050) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tssj8-0001qQ-Ia; Wed, 09 Jan 2013 05:17:42 -0500 Original-Received: by mail-da0-f52.google.com with SMTP id f10so673861dak.11 for ; Wed, 09 Jan 2013 02:17:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:subject:from:to:cc:date:in-reply-to :references:organization:content-type:x-mailer:mime-version :content-transfer-encoding; bh=oiyu+vabhec6qB8ODFpnSxp+dFtoU1+1wKFpdgBuWPY=; b=XwIgYeqK9Ww4WbB8e5uq5UjllGYRSypFJYhhCBQTk4+KZ9Tkt8z+1BTpjjzdlCwgW2 boM4Q3zgGo2hjW2XvFFWWVoQcUIxgoDoQ2kAPnbbR5Rtx3Hi7pINT63kuN1c4k2ZYO1h q00Bcf9EcGEN0SOANTFRgBJvphZYQyS8wJeBohKRWrO1pkJMpi50oPTXUtb8SoK6ko4E Zx8hFIY+6A84RCcLXcMXydKunnUZKJKrpHs4djASuqQnOyR0liqQHLFxnrhzMO6jNFJG YzWPeATrNIXWBggM7Eh1QAfYgQd7w05vPxBl2hkX3mp94CaeAiJaqZiLHzR62dOd6dVx MEVQ== X-Received: by 10.69.16.100 with SMTP id fv4mr207606134pbd.135.1357726660387; Wed, 09 Jan 2013 02:17:40 -0800 (PST) Original-Received: from [147.2.147.112] ([61.14.130.226]) by mx.google.com with ESMTPS id nf9sm41391508pbc.17.2013.01.09.02.17.36 (version=SSLv3 cipher=OTHER); Wed, 09 Jan 2013 02:17:38 -0800 (PST) In-Reply-To: <87623dvy88.fsf@gnu.org> X-Mailer: Evolution 3.4.4 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 209.85.210.52 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:15373 Archived-At: On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote: > Hi Nala, > > Thanks for your work! > > 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 in > effect. Would be nice to fix it. > Well, I'm not sure what's the mean of 'recursive REPL'? > Once in effect, the result is pleasant. :-) > I'm glad you like it. ;-D > > 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. > > 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? > 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. > > +(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. > > > +(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... > > +(define generate-color > > + (lambda (colors) > > + (let ((color-list > > + (remove not > > + (map (lambda (c) (assoc-ref *color-list* c)) colors)))) > > Use filter-map instead. > nice to know that~ > > +(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. > > +(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. > > +(define *pre-sign* > > + `((LIST . "(") > > + (PAIR . "(") > > + (VECTOR . "#(") > > + (BYTEVECTOR . "#vu8(") > > + (ARRAY . #f))) ;; array's sign is complecated. > > It’s complicated, so what? :-) > > The comment should instead mention that arrays get special treatment in > ‘pre-print’. > > > +(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 color) is the control > > Is that comment necessary here? Ah~thanks for pointing out, it's the obsolete design. > > + (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. ;-) > > +(define is-sign? > > + (lambda (ch) > > + (char-set-contains? char-set:punctuation ch))) > > Perhaps ‘delimiter?’ would be a better name? > Agreed~ > > +(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. I do considered a more efficient way to print simpler colorized-array, but I decide to print it like this finally. I believe a more clear array-print-result make users hate arrays less, since it's too complicated in Guile, though we don't have to use it in complicated way all the time. > > +;; I believe all end-sign is ")" > > +(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 “Write a closing > parenthesis...”. > > > +(define *custom-colorized-list* (make-fluid '())) > > It’s better to use SRFI-39 parameters (which are in core now). > Well, fluid is easier. ;-P > > +(define (class? obj) > > + (is-a? obj )) > > It’s enough to use ‘struct?’ since objects are structs. 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’t needed: it’s just the ‘else’ case. > Agreed~ > > +(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)) > ...)) > Hmm...if it's unnecessary, I prefer be lazy... > > +(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. Any other suggestion? > Also, the name is misleading. Should be called ‘data->token-color’ or > something like that. > Agreed~ > > +(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))" > > No Texinfo escapes in docstrings. > Agreed~ > Thanks, > Ludo’. It's here now: https://github.com/NalaGinrut/guile-colorized/blob/upstream/ice-9/colorized.scm And I'm waiting for any help to write the test-case. Thanks!