Hello Adam,

On Sat, 31 Mar 2018 14:00:25 -0400
Adam Massmann <massmannak@gmail.com> wrote:

> Hi,
> 
> This is a patch to add xapers (https://finestructure.net/xapers/) and
> dependencies. I followed the steps in Contributing: Submitting Patches,
> but this is my first time contributing so I apologize if I made any
> mistakes.

Welcome!  Thank you for your contribution.

> Thanks a lot for the work on Guix, I've really enjoyed using it.

Glad you've enjoyed it.

Just a few suggestions:

First, could you add a copyright line for yourself on each of the
affected files?

> ---
>  gnu/packages/python.scm | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
> index 9e038ef4f..f505f15b2 100644
> --- a/gnu/packages/python.scm
> +++ b/gnu/packages/python.scm
> @@ -13082,3 +13082,22 @@ file system events on Linux.")
>          (base32
>           "0svc9nla3b9145d6b7fb9dizx412l3difzqw0ilh9lz52nsixw8j"))
>         (file-name (string-append name "-" version ".tar.gz"))))))
> +
> +(define-public python-latexcodec
> +  (package
> +    (name "python-latexcodec")
[...]
> +    (synopsis "Lexer and codec to work with LaTeX code in Python")

Maybe leave out the "Lexer and codec to" bit so this becomes "Work with
LaTeX code in Python" to make the synopsis more approachable.


> 
> 
> From f506eb11811eef1461b382d6d3cbcc273e62ad3d Mon Sep 17 00:00:00 2001
> From: Adam Massmann <massmannak@gmail.com>
> Date: Sat, 31 Mar 2018 13:08:11 -0400
> Subject: [PATCH 2/3] gnu: Add python-pybtex.
> 
> ---
>  gnu/packages/python.scm | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
> index f505f15b2..056a05235 100644
> --- a/gnu/packages/python.scm
> +++ b/gnu/packages/python.scm
> @@ -13101,3 +13101,35 @@ file system events on Linux.")
>      (synopsis "Lexer and codec to work with LaTeX code in Python")
>      (description "Lexer and codec to work with LaTeX code in Python.")
>      (license license:expat)))
> +
> +(define-public python-pybtex
> +  (package
> +    (name "python-pybtex")
> +    (version "0.21")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (pypi-uri "pybtex" version))
> +       (sha256
> +        (base32
> +         "00300j8dn5pxq4ndxmfmbmycg2znawkqs49val2x6jlmfiy6r2mg"))))
> +    (build-system python-build-system)
> +    (native-inputs
> +     `(("python-nose" ,python-nose)))
> +    (inputs
> +     `(("python-latexcodec" ,python-latexcodec)
> +       ("python-pyyaml" ,python-pyyaml)
> +       ("python-six" ,python-six)))
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'check
> +           ;; hack, where did the tests go?
> +           (lambda _
> +             (zero? 0))))))

Could you use "#:tests? #f" instead, and expand on exactly why the
tests are not being run (seems there are none?).


> +(define-public xapers
> +  (package
> +    (name "xapers")
> +    (version "0.8.2")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append
> +             "https://finestructure.net/xapers/releases/xapers-"
> +             version ".tar.gz"))
> +       (sha256
> +        (base32
> +         "0ykz6hn3qj46w3c99d6q0pi5ncq2894simcl7vapv047zm3cylmd"))))
> +    (build-system python-build-system)
> +    (propagated-inputs
> +     `(("python-urwid" ,python-urwid)))
> +    (inputs
> +     `(("poppler" ,poppler)
> +       ("python" ,python)
> +       ("python-xapian-bindings" ,python-xapian-bindings)
> +       ("python-pycurl" ,python-pycurl)
> +       ("python-latexcodec" ,python-latexcodec)
> +       ("python-pybtex" ,python-pybtex)
> +       ("python-pyyaml" ,python-pyyaml)
> +       ("python-six" ,python-six)))
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-after
> +             'install 'install-doc

Put these on the same line as "add-after".

> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (bin (string-append out "/bin"))
> +                    (man1 (string-append out "/share/man/man1")))
> +               (mkdir-p bin)
> +               (mkdir-p man1)
> +               (copy-file "man/man1/xapers.1"
> +                          (string-append man1 "/xapers.1"))
> +               (copy-file "man/man1/xapers-adder.1"
> +                          (string-append man1 "/xapers-adder.1"))
> +               (copy-file "bin/xapers-adder"
> +                          (string-append bin "/xapers-adder"))))))))
                   ^

Use "install-file" here.  It also creates the target directory, so you
can remove the mkdir-p's above.

Otherwise looks good to me.  Could you send an updated patch?

Thanks,
`~Eric