Subject: Re: [bug#33185] [PATCH v2 1/2] gnu: Add patchwork.

From: Ricardo Wurmus <[email-masked]>
Date: Wed, 23 Jan 2019 10:28:53 +0100

Hi Chris,

thanks for the patch!

> * gnu/packages/patchutils.scm (patchwork): New variable.
[…]
> +         (replace 'check
> +           (lambda* (#:key tests? #:allow-other-keys)
> +             (or (not tests?)
> +                 (begin
> +                   (setenv "DJANGO_SETTINGS_MODULE" "patchwork.settings.dev")
> +                   (invoke
> +                    "python" "-Wonce" "./manage.py" "test" "--noinput")
> +                   #t))))

Maybe write this as

         (replace 'check
           (lambda* (#:key tests? #:allow-other-keys)
             (when tests?
               (setenv "DJANGO_SETTINGS_MODULE" "patchwork.settings.dev")
               (invoke
                "python" "-Wonce" "./manage.py" "test" "--noinput"))
             #t))

> +         (replace 'install
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out"))) […]

This phase might be less verbose if you let-bound the result of
(site-packages inputs outputs) at the beginning.  It would also be good
if there were more comments about what’s going on.  It’s not all obvious
(e.g. why “lib” is copied to “docs”).

> +               (simple-format #t "replacing template pwclient symlink")

Use “display” instead of “simple-format #t”?

> +         (add-after 'install 'install-hasher
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out")))
> +               (chmod (string-append (site-packages inputs outputs)
> +                                     "/patchwork/hasher.py")
> +                      #o555)
> +               (symlink (string-append (site-packages inputs outputs)
> +                                       "/patchwork/hasher.py")
> +                        (string-append out "/bin/hasher")))
> +             #t))

Here also consider simplifying with let.

> +         ;; Create a patchwork specific version of Django's command line admin
> +         ;; utility.
> +         (add-after 'install 'install-patchwork-admin
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out")))
> +               (mkdir-p (string-append out "/bin"))
> +               (call-with-output-file (string-append out "/bin/patchwork-admin")
> +                 (lambda (port)
> +                   (display "#!/usr/bin/env python3

Should this really say “#!/usr/bin/env python3”?

--
Ricardo