[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!
Re: [PATCH] Colorized REPL, Ludovic Courtès, 2013/01/11
Re: [PATCH] Colorized REPL, Daniel Hartwig, 2013/01/11
Re: [PATCH] Colorized REPL, Nala Ginrut, 2013/01/12
Re: [PATCH] Colorized REPL, Ludovic Courtès, 2013/01/12
Re: [PATCH] Colorized REPL, Nala Ginrut, 2013/01/26