Re: Patch file for colorize module

[Ricardo Wurmus]

Hi Sahithi,

> Please find the attachment. Hope I made it correct this time. :)

Thanks, this looks a lot better.  Some comments below:

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

This indicates that you’re doing development as the root user.  It’s
better to use a regular user and only switch to the root user when
absolutely needed.  For work on Guix you will not need to do anything as
the root user.

Also, you can tell git to use your name and email address by running
these commands:

    git config --global user.name "Sahithi Yarlagadda"
    git config --global user.email "address@hidden"

Any commits you make after that will be properly attributed to you.

> Date: Sat, 26 May 2018 17:34:23 +0530
> Subject: [PATCH] Added Colorize module and Copyrights line to ui

As you can see in the git history of the repository we use a peculiar
convention for describing the changes.  For this patch it would be
something like this:

--8<---------------cut here---------------start------------->8---
ui: Add support for colorization.

* guix/ui.scm (ansi-color-tables): New variable.
(color, colorize-string): New procedures.
--8<---------------cut here---------------end--------------->8---

> diff --git a/guix/ui.scm b/guix/ui.scm
> index 8d351607d..20fbf761f 100755
> --- a/guix/ui.scm
> +++ b/guix/ui.scm
> @@ -9,7 +9,9 @@
>  ;;; Copyright © 2015, 2016 Mathieu Lirzin <address@hidden>
>  ;;; Copyright © 2016 Roel Janssen <address@hidden>
>  ;;; Copyright © 2016 Benz Schenk <address@hidden>
> -;;;
> +;;; Copyright © 2013,2014 Free Software Foundation, Inc.

Please add a space after the comma.

> @@ -106,7 +108,9 @@
>              guix-warning-port
>              warning
>              info
> -            guix-main))
> +            guix-main
> +            color
> +            colorize-string))

Do we really need to export the “color” procedure or is it enough to
export “colorize-string”?  Do we even need to export “colorize-string”
at all or should we only export the soft port that applies colours to
some strings?

> @@ -1578,4 +1582,49 @@ and signal handling has already been set up."
>    (initialize-guix)
>    (apply run-guix args))
>
> +(define ansi-color-tables

I think that “ansi-color-tables” could be renamed to “color-table”.

> +(define (color . lst)

For every procedure we need to have a short docstring that describes the
desired behaviour in a complete sentence or two.  Could you provide a
short description of the “color” procedure?  What does it do to its
variable argument list “LST”?

> +(define (colorize-string str . color-list)

The same applies here.

I’d be happy if you could make these changes quickly and send an updated
patch.  Once I receive it I’ll push it to a branch “wip-sahithi” in the
repository.

Thanks in advance!

- - - - - - -

We’re still lacking code to actually use “colorize-string” anywhere.
Could you please prepare a patch or a series of patches that achieves
the following:

* add a soft port to (guix ui) that colorizes strings that match an
  internal list of regular expressions.  The initial list of regular
  expressions could be something like this:

  '("^starting phase.*"
    "^phase .* succeeded.*"
    "^phase .* failed.*")

  See the documentation for “string-match” for details.

  Initially, the port should print any matching string as just green.
  All other strings should not be modified at all.  This will become a
  little fancier in the future to allow use to apply different colours
  or formatting to different parts of the strings just like it is done
  by “guix-build-log-minor-mode” of the Emacs interface for Guix.  (If
  you haven’t played with that interface yet, please do so.)

* change “guix-package” in “guix/scripts/package.scm” such that it will
  use the new soft port.  Note that “current-build-output-port” in (guix
  store) is a so-called parameter, which can be modified dynamically
  using “parameterize”.  Take a look at “guix/scripts/build.scm” where
  “current-build-output-port” is parameterized to the void port when
  “--quiet” was passed (this causes “guix build” to print no build log).
  The change to “guix-package” should be very similar.

Does this sound like a good idea to you?

By using a soft port I think we can limit future work (e.g. hiding of
parts of the build output) to just the inner procedures and data
structures of the soft port.

Please try to give us short updates every two days or so; this way we
can ensure that any questions you may have can be addressed quickly.
Please also ask right away when anything is unclear or if the suggested
approach seems weird.

--
Ricardo