Re: [PATCH]: Rewrite CRAN importer.

[ludo@gnu.org (Ludovic Courtès)]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> So I rewrote the CRAN importer to do this:
>
>   * download the DESCRIPTION file for a given package
>   * break it up into a simple alist
>   * transform the alist into a package expression
>
> This is much simpler than the sxml hackery we did before and the code
> can be reused to write an importer for Bioconductor, a popular,
> versioned R package repository for bioinformatics packages.[1]

Sounds great!

> From f2455d19461d50c775360aafb60c104281f83483 Mon Sep 17 00:00:00 2001
> From: Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>
> Date: Thu, 3 Dec 2015 16:12:09 +0100
> Subject: [PATCH] import: cran: Parse DESCRIPTION instead of HTML.
>
> * guix/import/cran.scm (description->alist, safe-car, listify,
>   beautify-description, description->package): New procedures.
> (table-datum, downloads->url, nodes->text, cran-sxml->sexp): Remove
> proceduces.
> (latest-release): Use parsed DESCRIPTION instead of SXML.
> * tests/cran.scm: Rewrite to match importer.
> ---
>  guix/import/cran.scm | 270 +++++++++++++++++++++++++--------------------------
>  tests/cran.scm       | 189 +++++++++++++++---------------------
>  2 files changed, 214 insertions(+), 245 deletions(-)

Bonus points for making it shorter.  :-)

> +(define (safe-car maybe-pair)

Does it have airbags?

> +  (if (or (null? maybe-pair)
> +          (not maybe-pair))
> +      #f
> +      (car maybe-pair)))

Seriously, I think this is the wrong way to deal with that.

> +(define (package->cran-name package)
> +  "Given a Guix PACKAGE value, return the name of the R package on CRAN."
> +  (let* ((source-url (and=> (package-source package)
> +                            (compose safe-car origin-uri)))

I would change it to:

  (match (package-source package)
    ((? origin? origin)
     (match (origin-uri origin)
       ((url rest ...)
        (let ((end (string-rindex url #\_)) …) …))
       (_               #f)))
    (_ #f))

This is more verbose, but it makes the intent clearer IMO, and avoids
“wrong-type-arg #f” errors that would arise with the proposed code.

> +  (let ((url (string-append %cran-url name "/DESCRIPTION")))
> +    (call-with-temporary-output-file
> +     (lambda (temp port)
> +       (and (url-fetch url temp)
> +            (call-with-input-file temp
> +              (compose description->alist read-string)))))))

I think it’s best to use ‘http-fetch’ or ‘http-fetch/cached’ from (guix
http-client), which does not necessitate the creation of a temporary
file.

> +(define (beautify-description description)
> +  "Improve the package DESCRIPTION by turning a beginning sentence fragment
> +into a proper sentence and by using two spaces between sentences."

Excellent.  :-)

> +URL: http://gnu.org/s/my-example
> +Description: This is a long description
> +spanning multiple lines: and it could confuse the parser that
> +there is a colon : on the lines.
> +  And: this line continues the description.
> +biocViews: 0
> +SystemRequirements: Cairo (>= 0)
> +Depends: A C++11 compiler. Version 4.6.* of g++ (as
> +	currently in Rtools) is insufficient; versions 4.8.*, 4.9.* or
> +	later will be fine.

Too bad that this format is almost, but not quite, recutils.

The rest LGTM!

Can you send an updated patch?

Thank you!

Ludo’.