guile-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Colorized REPL


From: Nala Ginrut
Subject: Re: [PATCH] Colorized REPL
Date: Fri, 11 Jan 2013 18:40:59 +0800

On Fri, 2013-01-11 at 16:13 +0800, Daniel Hartwig wrote:
> On 11 January 2013 14:29, Nala Ginrut <address@hidden> wrote:
> > On Thu, 2013-01-10 at 16:19 +0800, Daniel Hartwig wrote:
> > I changed these:
> > string-in-color => colorize-string
> > display-string-in-color => colorized-display
> >
> > What do you think?
> 
> Nicer anway.
> 
> >> 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.
> >
> 
> I see.
> 
> >
> > But how to check if a REPL is already running?
> 
> I believe there was some fluid referenced by a previous version of
> activate-colorized.
> 
> >> Procedure arguments “data” which should rather be “obj”.
> >>
> >
> > Colorize an 'obj' is strange, I think 'data' is better.
> >
> 
> Not strange at all in the context of Scheme where this is a well
> established convention.  “obj” literally means “any object”, talking
> about Scheme objects (not GOOPS).  “data” is not commonly used and
> when it is, typically has a very specific meaning that does not
> include “any object”.  To read a procedure definition that uses the
> name “obj” I can immediately tell what types of values it expects
> (anything!), whereas “data” carries no particular hints at all.
> 
> In any Scheme documentation, such as the Guile manual or any Revised*
> Report on Scheme, there is almost exclusive use of the name “obj” for
> arguments like this.  See the R5RS section “Entry format”.
> 

OK~fixed.

> > But what's the expected result of "(write #u8(0 1 2)) (newline)"?
> 
> As I wrote: #u8(0 1 2).  I provided that line to demonstrate that
> colorize-it was producing different output.
> 
> >
> >> 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.
> 
> Yes.
> 
> > 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.
> 
> Regardless of the purpose, if it is shipped as part of Guile then it
> will be used.  It's output must be reliable.
> 
> Keep in mind that you can not anticipate everything people will do
> with code.  It takes on it's own life and you can only smile and watch
> it play :-)
> 
> >
> > 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.
> 
> Test cases do not have to be efficient, they must prove the real code.
>  By replacing the innermost procedure—colorize-the-string—with a
> testing variant you are no longer testing the real code, but something
> else.
> 

Yes, that's a good point, and the test case could move out of the module
itself.

> >> 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.
> 
> I suppose the original comments were not so clear.  It is not only the
> string but other members such as “data” that do not fit the concept of
> “colour scheme”.  Anyway, given that it is an internal type there is
> not much point to restructuring it all, except for pedantics.
> 

Well, if it's too uncomfortable, maybe it should rename to
"colorize-context". Anyway, I agree with your opinion, it's not so
important for an internal type.

> 
> >> when, if “colorize” produced strings, it could do something like this:
> >>
> 
> > 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~
> 
> Indeed.  The new loops are certainly an improvement.
> 
> Personally I would have avoided call-with-output-string in ->cstr and
> other low-level procedures, since now there are several calls in and
> out of string ports.  I suppose that is another early design decision
> that is not worth the effort to change.
> 

Yes, I could do that but taking other advantage of others functions is
better, and it introduced call-with-outout-string anyway.

> >
> >> >> > +(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.
> 
> You could inspect the array and generate the tag yourself.  This would
> not explode on very large data structures.  But, as you go on to say …
> 

Seems it's the only 'suck point' now, I do try to handle the prefix with
Guile's function, but the Array is too complicated (and not the srfi-63
compatible), finally I lost, so I tried a easy solution...
PS: I don't think srfi-63 is matter here, just to say it's a brand new
complicated thing to learn...

> > 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.
> 
> Indeed, with this outlook there is little point to such alternative
> treatment, and the easiest thing to do is to rely on the existing
> procedures to produce the correct output.
> 

If the Array is the only problem, we'd better fix it anyway. I think
there's no other bad thing left, or not?

> However I will say that an efficient and flexible implementation would
> certainly be useful in applications outside of the developers REPL.
> Applications display data, and often contain REPL and equivalent. :-)
> 
> > Please review it:
> > https://github.com/NalaGinrut/guile-colorized/tree/upstream
> >
> 
> This:
> 
>  (define colorize-the-string
>    (lambda (color str control)
>      (string-append "\x1b[" (generate-color color) "m" str
>        "\x1b[" (generate-color control) "m")))
> 
> Why move "\x1b[" and "m" out of generate-color here?  Have that
> procedure procedure the /complete/ escape sequence and it is much
> neater.
> 

Ah~yeah, fixed.

> Regards
> 

I'll call this 'sculpture hacking', hah? ;-D


Thanks!





reply via email to

[Prev in Thread] Current Thread [Next in Thread]