Thanks for the heads up.  

I'm not sure that all your suggestions will work, but I will try what you suggest
and push follow-up commit.

Regarding indentation - I normally let emacs handle it. Shouldn't that get everything
correct?

J'


On Sun, Mar 19, 2017 at 02:11:51PM +0100, Ricardo Wurmus wrote:
     
     Hi John,
     
     thanks for the contribution.
     
     > * gnu/packages/image.scm (niftilib): New variable.
     
     I realize this has been pushed already, but here are some comments for
     the future.
     
     > +
     > +(define-public niftilib
     > +  (package
     > +   (name "niftilib")
     > +   (version "2.0.0")
     > +   (source (origin
     > +            (method url-fetch)
     > +            (uri (list (string-append "mirror://sourceforge/niftilib/"
     > +                                      "nifticlib/nifticlib_2_0_0/"
     > +                                      "/nifticlib-" version ".tar.gz")))
     > +            (sha256
     > +             (base32 "123z9bwzgin5y8gi5ni8j217k7n683whjsvg0lrpii9flgk8isd3"))))
     > +   (build-system gnu-build-system)
     > +   (arguments
     > +    '(#:tests? #f
     > +      #:parallel-build? #f
     
     Please add simple comments explaining why this is required.
     
     > +      #:phases
     > +      (modify-phases %standard-phases
     > +        (replace 'install
     > +                 (lambda _
     > +                   (for-each
     > +                    (lambda (dir)
     > +                      (let ((directory (assoc-ref %outputs "out")))
     > +                        (mkdir-p (string-append directory "/" dir))
     > +                        (zero? (system* "cp" "-a" dir directory))))
     > +                    '("bin" "lib" "include"))))
     
     Instead of starting a shell process to copy things please use
     ???copy-recursively??? or ???install-file??? instead.  It???s also better style to
     pull the let binding out of the lambda.
     
     Finally, please end the phase on ???#t???.
     
     > +        (replace 'configure
     > +                 (lambda _
     > +                   (substitute* "Makefile"
     > +                     (("^SHELL[ \t]*=[ \t]*csh")
     > +                      (string-append "SHELL = "
     > +                                     (assoc-ref %build-inputs "bash")
     > +                                     "/bin/sh"))
     > +
     > +                     (("^CFLAGS[ \t]*=[ \t]\\$\\(ANSI_FLAGS\\)")
     > +                      "CFLAGS = $(ANSI_FLAGS) -fPIC")
     > +
     > +                     (("^ZLIB_INC[ \t]*=[ \t]*-I/usr/include")
     > +                      (string-append "ZLIB_INC = -I"
     > +                                     (assoc-ref %build-inputs "zlib")
     > +                                     "/include"))
     > +
     > +                     (("^CP[ \t]*=[ \t]*cp")
     > +                      (string-append "CP = "
     > +                                     (assoc-ref %build-inputs "coreutils")
     > +                                     "/bin/cp")))
     > +                   #t)))))
     
     It is preferrable to use #:make-flags instead of patching the Makefile.
     When referencing inputs, please use ???lambda* (#:key inputs
     #:allow-other-keys)??? instead of ???lambda _??? and ???%build-inputs???.
     
     It would also be good to check the indentation, which can be done with
     ???etc/indent-code.el???.
     
     --
     Ricardo
     
     GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
     https://elephly.net

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.