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

-Steve

On Sat, Aug 15, 2015 at 10:07 PM, Ricardo Wurmus <rekado@elephly.net> 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 <scs@stevesprang.com>
> > 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
>
>