Ludovic Courtès <ludo@gnu.org> writes:

>> +(define (safe-car maybe-pair)
>
> Does it have airbags?

Rounded corners, no explosive combustion engine, and wrapped in an extra
thick layer of fluff!

>> +  (if (or (null? maybe-pair)
>> +          (not maybe-pair))
>> +      #f
>> +      (car maybe-pair)))
>
> Seriously, I think this is the wrong way to deal with that.

Okay.  I’m using ‘match’ instead.  It’s also shorter and clearer.

>> +(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.

Yes, this is better.  I’ve also made this private as it is not needed
anywhere else.

>> +  (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.

Okay.

Attached is a new patch (I didn’t resend the first patch adding new
license conversions).