Re: [PATCH] gnu: Add figlet.

[Steve Sprang]

Thanks for the feedback! Here's my 2nd attempt.

-Steve

On Sat, Aug 15, 2015 at 10:07 PM, Ricardo Wurmus <address@hidden> wrote:

Hi Steve,

thank you for your first Guix package!  It looks great!  I do have a
couple of cosmetic comments, though.

> From 1abd103363bb12e7b8aa5f5ad1329c94b0af48e8 Mon Sep 17 00:00:00 2001
> From: Steve Sprang <address@hidden>
> Date: Sat, 15 Aug 2015 20:08:38 -0700
> Subject: [PATCH] gnu: Add figlet.

> * gnu/packages/figlet.scm: New file.
> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.

Looks good.  I just wonder if figlet could not be added to some existing
module instead of creating a new one.  Maybe “fontutils.scm”?

> +    (version "2.2.5")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append
> +             "ftp://ftp.figlet.org/pub/figlet/program/unix/figlet-" version ".tar.gz"))

This line is a bit long.  How about this instead:

       (uri (string-append "ftp://ftp.figlet.org/pub/figlet/program"
                           "/unix/figlet-" version ".tar.gz"))

> +       (sha256 (base32
> +                "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))))

I think it would look better like this:

       (sha256
        (base32 "0za1ax15x7myjl8jz271ybly8ln9kb9zhm1gf6rdlxzhs07w925z"))

> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:phases
> +       (alist-replace
> +        'configure
> +        (lambda* (#:key outputs #:allow-other-keys)
> +          (let ((out (assoc-ref outputs "out")))
> +            (substitute* "Makefile"
> +              (("/usr/local") out))))
> +        %standard-phases)))

I suggest using ‘(modify-phases ...)’ instead, as adding or removing
phases later on does not alter the indentation of other phases.

However, in this case ‘/usr/local’ doesn’t have to be patched away at
all.  You could just pass a make-flag to set ‘prefix’ to ‘(assoc-ref
outputs "out")’.

> +    (synopsis "Program for making large letters out of ordinary text")
> +    (description "FIGlet is a program for making large letters out of ordinary text.")

This line is too long.  You can automatically format it in Emacs with
‘M-q’.  Maybe the description could be a little longer to explain that
what figlet generates is some sort of ASCII art letters, because this
description could be misunderstood as operating on fonts.  Or maybe it’s
just me being dense ;)

> +    (home-page "http://www.figlet.org/")
> +    (license bsd-3)))

Looking at the sources it looks like not all files are under the BSD-3
license.  ‘inflate.c’, for example, is released under expat/X11;
‘getopt.c’ is public domain software — I’m not sure if this warrants
using a list for the license field.  Maybe someone else could weigh in
on this.

~~ Ricardo

add-figlet-2.patch
Description: Text Data