Re: Fwd: Re: Patch file for colorize module

[Ricardo Wurmus]

Hi Sahithi,

> I have made necessary changes to ui, patch file is attached. Please
> review changes and additionals to be added.

Thank you.  Please see my inline comments below.

> From ab1d07107b3f701b8bcaed8eab3565dc0968f20c Mon Sep 17 00:00:00 2001
> From: root <address@hidden>

Please change the author name and email address to your name and email
address.  I’ve previously sent you instructions on how to do this using
“git config”.  (Generally, you really shouldn’t be developing things as
“root”.)

> Date: Tue, 5 Jun 2018 00:08:32 +0530
> Subject: [PATCH] Added a soft port to (guix ui) that colorizes strings that
>  match Regular Expressions.
>

Please follow the conventions for commit messages.  If you take a look
at previous messages you can learn the style.

For this commit a message like this would be appropriate:

--8<---------------cut here---------------start------------->8---
guix: Add coloring soft port.

* guix/ui.scm (foo, bar): New procedures.
(guix-colorful-port): New variable.
--8<---------------cut here---------------end--------------->8---

It should mention all additions by name.

> diff --git a/guix/ui.scm b/guix/ui.scm
> index 80f1a4d77..3a36daadc 100644
> --- a/guix/ui.scm
> +++ b/guix/ui.scm
> @@ -109,7 +109,8 @@
>              warning
>              info
>              guix-main
> -            colorize-string))
> +            colorize-string

We may not need to export “colorize-string” at all.

> +(define target-port (current-output-port))

Please remove this and use “(current-error-port)” instead of
“target-port” below.

> +(define (handle-string str)

Please always add a so-called docstring for all defined procedures.

> +   (or (and=> (string-match "^(starting phase)(.*)" str)
> +           (lambda (m)
> +             (string-append
> +               (colorized-display (match:substring m 1) '(BLUE))
> +               (colorized-display (match:substring m 2) '(GREEN)))))
> +
> +       (and=> (string-match "^(phase)(.*) (succeeded)(.*)" str)
> +          (lambda (m)
> +            (string-append
> +              (colorized-display (match:substring m 1) '(BLUE))
> +              (colorized-display (match:substring m 2) '(GREEN))
> +              (colorized-display (match:substring m 3) '(BLUE))
> +              (colorized-display (match:substring m 4) '(GREEN)))))
> +
> +       (and=> (string-match "^(phase)(.*) (failed)(.*)" str)
> +          (lambda (m)
> +            (string-append
> +              (colorized-display (match:substring m 1) '(BLUE))
> +              (colorized-display (match:substring m 2) '(GREEN))
> +              (colorized-display (match:substring m 3) '(BLUE))
> +              (colorized-display (match:substring m 4) '(GREEN)))))
> +
> +    ;; Didn’t match, so return unmodified string.
> +      str)

This does not do what you may think it does.  This big “or” expression
returns a string, but you are not using the return value anywhere.  See
the next line:

> + (display str target-port))

No matter what string you built up above, you are simply pushing the
*original* string to “target-port”.  That’s not correct.

> +(define guix-colorful-port
> +  (make-soft-port
> +   (vector
> +    (lambda (c) (write c target-port))
> +    handle-string
> +    (lambda () (display "." target-port))
> +    (lambda () (char-upcase (read-char)))
> +    (lambda () (display "@" target-port)))
> +   "rw"))
> +

This is okay, but please don’t name it “guix-colorful-port” — everything
here is about Guix anyway ;)  I can’t think of a good name right now,
but “colorful-build-output-port” seems okay.

Please add a docstring to explain what this port is used for.

Let’s go back to the code in “handle-string”.  Let’s first look at the
regular expressions:

> +   (or (and=> (string-match "^(starting phase)(.*)" str)
[…]
> +       (and=> (string-match "^(phase)(.*) (succeeded)(.*)" str)
[…]
> +       (and=> (string-match "^(phase)(.*) (failed)(.*)" str)

The strings come from the “gnu-build-system” (and its derivatives).  They
are produced in “(guix build gnu-build-system)”.

We see there that the format strings look like this:

    "starting phase `~a'~%"
    "phase `~a' ~:[failed~;succeeded~] after ~,1f seconds~%"

So the strings look something like this:

    "starting phase `configure'"
    "phase `install' succeeded after 3.2 seconds"
    "phase `patch-generated-file-shebangs' failed after 3.2 seconds"

Keep this in mind if you want to change the expressions in the future.

I have attached a screenshot of the colours that the Emacs interface
produces here.  Can you update the regular expressions so that they also
match the number of seconds?  Then we can give them a different colour.

> +           (lambda (m)
> +             (string-append
> +               (colorized-display (match:substring m 1) '(BLUE))

I don’t see a definition for “colorized-display”.  In your previous
commit you only added “colorize-string”, which is the right tool for
this job as it returns a new colourful string.

> +       (and=> (string-match "^(phase)(.*) (failed)(.*)" str)
> +          (lambda (m)
> +            (string-append
> +              (colorized-display (match:substring m 1) '(BLUE))
> +              (colorized-display (match:substring m 2) '(GREEN))
> +              (colorized-display (match:substring m 3) '(BLUE))
> +              (colorized-display (match:substring m 4) '(GREEN)))))

Let’s use red when a phase failed.

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?  :)

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?

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.

Then use “guix package -i” or “guix build” to build and install a
package.  Also add a broken package definition to ensure that the
“failed” messages become red.  Before the internship you added some R
packages, so I think you are already sufficiently familiar with writing
package definitions.

If you have questions about any of this, please ask the friendly people
on the #guix IRC channel for help.

2018-06-05-212956_1920x1080_scrot.png
Description: PNG image

--
Ricardo