Subject: Re: bug#25733: [PATCH 01/18] gnu: Add python-geventhttpclient.

From: Ricardo Wurmus <[email-masked]>
Date: Sat, 18 Feb 2017 12:37:54 +0100

Hi ng0,

> +(define-public python-geventhttpclient
> +  (package
> +    (name "python-geventhttpclient")
> +    (version "1.3.1")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (pypi-uri "geventhttpclient" version))
> +       (modules '((guix build utils)))
> +       (snippet
> +        '(begin
> +           ;; https://github.com/gwik/geventhttpclient/pull/82
> +           (delete-file-recursively
> "src/geventhttpclient/tests/__pycache__")))

Please end the snippet with “#t”.  Please also extend the comment,
e.g. “Remove bundled foo.  Frobnicate the bar.  See https://…”.

> +       (sha256
> +        (base32
> +         "07d0q3wzmml75227r6y6mrl5a0zpf4v9gj0ni5rhbyzmaj4az1xx"))))
> +    (build-system python-build-system)
> +    (inputs
> +     `(("python-certifi" ,python-certifi)
> +       ("python-gevent" ,python-gevent)
> +       ("python-six" ,python-six)
> +       ("python-pytest" ,python-pytest)
> +       ("python-pytest-runner" ,python-pytest-runner)))

These should not be inputs but propagated inputs like all other packages
providing Python libraries.  Python *applications* are wrapped, so they
don’t need propagation, but libraries still do.

> +    (home-page "https://github.com/gwik/geventhttpclient")
> +    (synopsis "Http client library for gevent")

What is “gevent”?  Could this be made more obvious by saying e.g. “for
the gevent framework” (I don’t know if it is a framework)?

Please spell “Http” as “HTTP”.

> +    (description
> +     "High performance, concurrent HTTP client library for Python
> using gevent.")

Please provide a full description, i.e. using full sentences.

> +    (license (list license:expat
> +                   ;; http_parser.c is based on src/http/ngx_http_parse.c
> +                   ;; from NGINX copyright Igor Sysoev
> +                   license:bsd-3))

The comment referencing nginx is not needed, but a comment above the
license field is needed that explains why there are two licenses.  If
only “http_parser.c” is under BSD 3 then please put that in the comment
above the license field.

> +    (properties `((python2-variant . ,(delay python2-geventhttpclient))))))

Why is this needed?

> +(define-public python2-geventhttpclient
> +  (package
> +    (inherit (package-with-python2
> +              (strip-python2-variant python-geventhttpclient)))))
> +

Same question as above: why is “package-with-python2” not enough in this
case?

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net