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 <nalagin...@gmail.com> wrote:
> > On Fri, 2013-01-04 at 15:06 +0100, Ludovic Courtès wrote:
> >> Nala Ginrut <nalagin...@gmail.com> 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>
> >> (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>
> >> (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!
