Re: Fwd: Re: Patch file for colorize module

[Ricardo Wurmus]

Hi Sahithi,

> Please find my changes in the patch file attached.

Thanks.  Could you please squash these changes and the previous commit?
This is easier for me to review.

Thanks for fixing the commit message!

>> You have probably noticed that this looks rather repetitive at this
>> point.  Maybe we can think of a better way to express what colours
>> should be applied.  The match group numbers are monotonically
>> increasing, so maybe we can avoid repeated statements of this kind and
>> simply iterate over a list of colours…  I have an idea already; how
>> about you?  :)
>
> I have an idea about making a using filter-string and lists. Not sure
> about functionality but that seems fine. :-P

Could you share some more details?  It doesn’t have to be a ready
implementation in Scheme, but I’d love to hear a few more details.

>> Another thing that’s worth thinking about now is the next step:
>> how can we *optionally* hide all lines between these build system
>> notices about started and completed build phases?
> I din't think of it yet. Will do it in mean process.

That’s okay.  Let’s concentrate on the matter at hand first.

>> One more thing: the fact that handle-string didn’t do the right thing
>> indicates that you didn’t test the changes well enough.  To test them,
>> please locally modify “guix/scripts/build.scm” and/or
>> “guix/scripts/package.scm” to make it use your colourful port instead of
>> the default, as discussed on IRC and in previous emails.
> I made changes to /guix/scripts/build.scm /but that din/t workout. That
> resulted me colourless outputs as before. :-(

That’s because you must have overlooked a comment I made about your code
in “handle-string” :)  Let’s look at it:

>  (define (handle-string str)
>     (or (and=> (string-match "^(starting phase)(.*)" str)
[…]
>      ;; Didn’t match, so return unmodified string.
> -      str)
> - (display str target-port))
> +       (display str current-error-port)))

This suffers from the same problem as before.  The result of the
expression starting with “or” is ignored.  You don’t do anything with
it.  After that expression you just run “(display str
current-error-port)”, which prints the original “str” to the (undefined)
“current-error-port”.

You should do something with the result of the previous expression, such
as storing it in a variable.  Then you can use that variable in the
final line.  Something like this:

--8<---------------cut here---------------start------------->8---
(define (handle-string str)
  (let ((message (or (and=> …)
                     …
                     ;; Nothing matched, use unmodified string.
                     str)))
    (display message (current-error-port))))
--8<---------------cut here---------------end--------------->8---

Also note that you must use “(current-error-port)”, not
“current-error-port”.

Could you please send an updated patch?

--
Ricardo