[bug#41919] [patch] Add curlpp C++ bindings to curl package

[Marius Bakke <marius@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]

Hello Dale,

Thanks for the updated patch.  I will suggest another few improvements,
then I think it should be good to go.  :-)

Dale Mellor <guix-devel-0brg6b@rdmp.org> writes:

> From b1506404e27552a171fe535144b2d11b6aa67cfa Mon Sep 17 00:00:00 2001
> From: Dale Mellor <guix-devel-0brg6b@rdmp.org>
> Date: Sun, 21 Jun 2020 23:53:07 +0100
> Subject: [PATCH] gnu: Add curlpp library.
>
> * gnu/packages/curl.scm (curlpp): New variable.

[...]

> @@ -32,10 +33,12 @@
>    #:use-module (guix download)
>    #:use-module (guix git-download)
>    #:use-module (guix utils)
> +  #:use-module (guix build-system cmake)
>    #:use-module (guix build-system gnu)
>    #:use-module (guix build-system go)
>    #:use-module (gnu packages)
>    #:use-module (gnu packages compression)
> +  #:use-module (gnu packages gawk)

This import is unused.

>    #:use-module (gnu packages golang)
>    #:use-module (gnu packages guile)
>    #:use-module (gnu packages kerberos)
> @@ -258,3 +261,33 @@ not offer a replacement for libcurl.")
>  Guile to do client-side URL transfers, like requesting documents from HTTP or
>  FTP servers.  It is based on the curl library.")
>     (license license:gpl3+)))
> +
> +(define-public  curlpp
                 ^^
Extra space _____|

> +  (package
> +   (name "curlpp")
> +   (version "0.8.1")
> +   (source
> +    (origin
> +             (method  git-fetch)

What happened with the indentation here?  Also, there is an extra space
between 'method' and 'git-fetch'.

> +     (uri
> +      (git-reference

Place git-reference on the same line as uri.

> +                   (url "https://github.com/jpbarrette/curlpp.git")
> +                   (commit "v0.8.1")))

Use (string-append "v" version) here instead of hard-coding.  Also, you
should add a (file-name (git-file-name name version)) in this vicinity.

> +             (sha256
> +              (base32 "1b0ylnnrhdax4kwjq64r1fk0i24n5ss6zfzf4hxwgslny01xiwrk"))))
> +   (build-system  cmake-build-system)
                   ^^
Another extra space.

> +   ;; There are no build tests to be had.
> +   (arguments
> +    '(#:tests? #f))
> +   ;; The installed version needs the header files from the C library.
> +   (propagated-inputs
> +    `(("curl" ,curl)))
> +   (synopsis  "C++ wrapper around libcURL")
               ^^
Here too ______|

> +   (description
> +    "A free and easy-to-use client-side C++ URL transfer library,

Please use full sentences in the description.  In particular, start with
"This package provides a ..." instead of jumping to the "A ...".

> +supporting FTP, FTPS, HTTP, HTTPS, GOPHER, TELNET, DICT, FILE and LDAP; in
> +particular it supports HTTPS certificates, HTTP POST, HTTP PUT, FTP uploading,
> +kerberos, HTTP form based upload, proxies, cookies, user+password
> +authentication, file transfer resume, http proxy tunneling and more!")
> +   (home-page  "http://www.curlpp.org")
> +   (license  license:x11-style)))

There are extra spaces after 'home-page' and 'license'.  Also,
license:x11-style is a procedure that takes two arguments: an URI and
optionally a comment.  However reading the source files, it looks like
the standard "Expat" license, so you can use license:expat instead.

Can you send an updated patch?  TIA!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]