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: Fri, 11 Jan 2013 14:29:09 +0800 Organization: HFG Message-ID: <1357885749.2798.163.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> <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: 8bit X-Trace: ger.gmane.org 1357885767 3152 80.91.229.3 (11 Jan 2013 06:29:27 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 11 Jan 2013 06:29:27 +0000 (UTC) Cc: guile-devel@gnu.org To: Daniel Hartwig Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Jan 11 07:29:44 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 1TtY7c-0003th-6p for guile-devel@m.gmane.org; Fri, 11 Jan 2013 07:29:44 +0100 Original-Received: from localhost ([::1]:42732 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtY7M-0007NE-42 for guile-devel@m.gmane.org; Fri, 11 Jan 2013 01:29:28 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:39649) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtY7G-0007Mx-SJ for guile-devel@gnu.org; Fri, 11 Jan 2013 01:29:26 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TtY7D-0003aY-Nb for guile-devel@gnu.org; Fri, 11 Jan 2013 01:29:22 -0500 Original-Received: from mail-pa0-f46.google.com ([209.85.220.46]:54016) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtY7D-0003Xi-9K for guile-devel@gnu.org; Fri, 11 Jan 2013 01:29:19 -0500 Original-Received: by mail-pa0-f46.google.com with SMTP id bh2so827341pad.19 for ; Thu, 10 Jan 2013 22:29:16 -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=e81DL3IzP0it9S0U3Rr7X+t2y6eub06X+lhQdfxOwzQ=; b=CGo6AiZf5Klmtec4cffxwnp2MBFvcF0QSEZX2N8Fbb6G58WHA8kwB2qVQab8tm0bmA z1V9Z7Eb8ZlG7aketJqvs5O+M0BAWa6Ne5u9nhcVHCGEYFvyfpGlQiCjVdu+lLe8onh3 j9MVezeuQXCG5TPn/ZgZsn+hYUn5BHizOIlkIt9zbyckpyjVl9gNVEj0rsBf8GhaGaw+ SXjNdFIkN8aNjGn+xiTDDQzU0NCnTewz/51BGDI85bZ03xGLXWF2ZlRxu5GD7FSl6ikM mb+B99gncLPOr5dF9Kl9d6kdYMrCDjXuAQG8q5xat1DFTPeJhrYe3EtFlM4Q8AWZ8L9N jzDQ== X-Received: by 10.66.84.232 with SMTP id c8mr23458460paz.8.1357885755963; Thu, 10 Jan 2013 22:29:15 -0800 (PST) Original-Received: from [147.2.147.112] ([61.14.130.226]) by mx.google.com with ESMTPS id bc1sm2513161pab.3.2013.01.10.22.29.12 (version=SSLv3 cipher=RC4-SHA bits=128/128); Thu, 10 Jan 2013 22:29:14 -0800 (PST) In-Reply-To: X-Mailer: Evolution 3.4.4 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 209.85.220.46 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:15380 Archived-At: On Thu, 2013-01-10 at 16:19 +0800, Daniel Hartwig wrote: > Hello again > > Some comments in addition to Ludo's below. I have not inspected the > code of your latest submission thoroughly, but enough to agree that > there are many stylistic and algorithmic issues. I will probably not > be looking in to it any more, and remain a satisfied user of > emacs+geiser. > > I still think that this data colourizing or whatever is the domain of > third-party packages, rather than something to include in Guile > proper. > Well, as I mentioned before, not all people use Emacs, and many of them, especially newbies, they may never tried Emacs/vi. > > > string-in-color > > colorize-string is a much nicer name. > Agreed~ I changed these: string-in-color => colorize-string display-string-in-color => colorized-display What do you think? > > enable-color-test disable-color-test > > These should not be exported. > > > colorize > > This is the most useful procedure in the module, it should really be exported. > Alright, fixed. > > The default colour scheme is far too aggressive and not to my taste at > all. The focus should be on highlighting the structure (i.e. syntax), > but that is hard to spot when practically every data type has it's own > tweaked colours. Highlighting is subtle, only a very few conditions > should change the colour: strings and parens are great, but please > leave numbers in whatever the current colour is. > > That said, a colour scheme for testing should probably be quite > aggressive, to the point of giving every condition it's own unique set > of attributes. > > Also, the “/” in “1/2” appears as a different colour, why?! > It's more conspicuous for the users. I asked some guys, they like it. > > On 9 January 2013 18:17, Nala Ginrut wrote: > > On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote: > >> Nala Ginrut skribis: > >> > (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'? > > Starting one REPL inside another. The updated activate-colorized only > sets default REPL options and does not take care to update the current > instance if one is already active. So, if an REPL is already running, > one has to do (activate-colorized) followed by starting a new REPL. > But how to check if a REPL is already running? > >> 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. > > Procedure arguments “data” which should rather be “obj”. > Colorize an 'obj' is strange, I think 'data' is better. > >> > >> 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. > > Yes this is quite concerning. Having less implementations is > certainly preferable to having more. > > The current output is broken for some data types: > > scheme@(ice-9 colorized)> (colorize-it #f32(0 1 2)) > #vu8(0 0 0 0 0 0 128 63 0 0 0 64) > scheme@(ice-9 colorized)> (colorize-it #u8(0 1 2)) > #vu8(0 1 2) > scheme@(ice-9 colorized)> (write #u8(0 1 2)) (newline) > #u8(0 1 2) > It's an important correction, I realized that I don't have to handle bytevector, it's an special array too, and I don't have to import (rnrs bytevectors), thanks for pointing out it! I believe the bugs above all fixed now. But what's the expected result of "(write #u8(0 1 2)) (newline)"? > The chance of subtle problems with other data types is high, both now > and in the future after any current problems are corrected. > Any code contains potential bugs. I don't think people should use (ice-9 colorize) in their serious program, it's just a tool for newbies to learn Guile more friendly. Even 'colorize-string', it's the co-product of that, and it just output a string in color, simple enough to avoid dangerous. > >> Could you > >> add test cases that compare the output of both, for instance using a > >> helper procedure that dismisses ANSI escapes? > > You have provided: > > > (define color-it-test > > (lambda (color str control) > > str)) > > rather, you want to write a procedure that takes a string with ANSI > code sequences embedded and removes the ANSI codes so that only plain > text remains. That plain text can then be compared with the output > from write/display. > > (define (remove-ansi-codes str) …) > > then use that in the test-cases such as: > But it's inefficient if I remove ansi-code each time after it's generated. I prefer output the plain string on the fly with enable test option. > (define (compare-write-and-colorize obj) > (string= (with-output-to-string > (lambda () (write obj))) > (remove-ansi-codes > (with-output-to-string > (lambda () (colorize obj)))))) > > but structure your test cases as per the existing test-suite. > > > 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 the existing tests filed under test-suite/tests. > > >> 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... > > Right, lists are more natural for specifying these sets of attributes, > which could be any combination of foreground, background, and/or > something other. (ansi term) module sets a very respectable example. Anyway, I'll keep it. > > >> > +(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. > > I agree with Ludo, the string and color scheme are not so related. > This design choice adds confusion to the rest of the module. > OK, I think it's an absolute design, fixed. > > > >> > +(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. > > Regardless, it is poor style to produce something only to subsequently > scrub it out. Code does this: > > + (for-each (lambda (x) (colorize x port) (space port)) data) > + (backspace port) ;; remove the redundant space > > when, if “colorize” produced strings, it could do something like this: > > (display (string-join (map colorize data) " ") port) > > or, perhaps more efficiently, this: > > (format port "~{~a~^ ~}" (map colorize data)) > Nice~I'm in the first way, and added a helper function '->cstr' to generate color string result for any type. A good hack I like it~ > >> > +(define color-array-inner > > >> 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. > > Disagree. It is more difficult to read the array tag with all the > colour changes. Breaking apart elements to this level is extreme, > prone to errors, and poorly maintainable. All that for a very > questionable gain in “friendly UI”. Keep things simple, at least > until you have a smooth module without issues. > Even if I don't break apart it, it's inefficient too, I have to convert it to string then output the prefix-part. As I said, nobody will use it in one's serious program, since it's only about REPL. Who care about the REPL running speed? Users like a more friendly REPL UI not a quick REPL since it's useless in a released program but for test/debug. Except for 'colorized-string', but it's a simple function which is safe and fast. > Also, this colours the vectag (such as “s16” or “u8”) for arrays, but > does not do the same for bytevectors. This comes across as very > inconsistent, especially with two such values next to each other. > Just leave the array tags in a single colour. I removed bytevectors since it's unnecessary. > > >> > +(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 > > >> > >> 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. > > You might want to reread the definition of “any”. > Right~it's a nice thing but I misunderstood it. Fixed. > Best wishes. Please review it: https://github.com/NalaGinrut/guile-colorized/tree/upstream It become better and better now, no matter if it's applied, it's a nice thing to play, and I learned many things from this work. Thanks guys!