Hi Marius,

On Mon, Apr 16, 2018 at 11:36:56PM +0200, Marius Bakke wrote:
>Tomáš Čech <sleep_walker@gnu.org> writes:
>
>>  * gnu/packages/python.scm (python2-urlgrabber): New variable.
>
>There is a 'python-web' module as well, would that be a good fit for
>this package?  python.scm had to be split up since it was causing
>problems for the Guile compiler.

I believe that it will fit into python-web as well.

>
>[...]
>
>> +(define-public python2-urlgrabber
>> +  (package
>> +    (name "python2-urlgrabber")
>> +    (version "3.10.2")
>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri (pypi-uri "urlgrabber" version))
>> +       (sha256
>> +        (base32
>> +         "0w1h7hlsq406bxfy2pn4i9bd003bwl0q9b7p03z3g6yl0d21ddq5"))))
>> +    (build-system python-build-system)
>> +    (arguments
>> +     `(#:python ,python-2))
>
>If Python 3 is unsupported, please add a comment about it.  Otherwise
>it's better to add a Python 3 variant and make the Py2 variant inherit
>from it.

Yes, python 2 is the only supported, I'll add comment.

>
>> +    (home-page "http://urlgrabber.baseurl.org")
>> +    (synopsis "A high-level cross protocol url-grabber")
>
>Synopses should not start with an article (I believe `guix lint` will
>warn about this).  Simply "High-level cross-protocol URL grabber".

I'll use that.

>
>> +    (description "URL grabber library unifying http://, ftp:// and file://
>> +protocols access, supporting HTTP keepalive, reget, throttling and other
>> +convenience features.")
>
>Please turn this into one or more complete sentences.
>E.g. "@code{urlgrabber} is a Python library that unifies access to
>...  It supports bla bla.".

To be honest, this is always the most painful part for me. I'll try to come with something better.

>
>> +    (license license:lgpl2.1)))
>
>This should be LGPL2.1+ as mentioned in the code headers.

I'll fix that.


Thanks for review, I'll prepare next version.

Best regards,

S_W