Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Chris,
>
> thanks for the patch!

And thanks for your feedback :) I've sent some updated patches.

>> * 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))

Yep, I've updated it to use when now.

>> +         (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”).

I've used let more now, and tried to be a bit more descriptive in the
comments. I'll reconfigure the patchwork instances I host at some point
and hopefully add more detail then.

>> +               (simple-format #t "replacing template pwclient symlink")
>
> Use “display” instead of “simple-format #t”?

That was just a debugging thing, I've removed it.

>> +         (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.

Yep, I've used let here more now.

>> +         ;; 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”?

So this was fine, as it was replaced in a future stage. But yes, it's
probably better to use a more appropriate value that isn't changed later
on. I've updated this now.

So yes, I've sent some updated patches, still a few niggles, but they're
getting better.