Re: [PATCH 1/5] import: Add 'elpa' importer

[Federico Beffa]

Thanks for the review!

I've made the suggested changes, except for the comments below.

On Wed, Jun 24, 2015 at 11:06 PM, Ludovic Courtès <address@hidden> wrote:
> Federico Beffa <address@hidden> skribis:
>> +Specific command-line options are:
>> +
>> address@hidden @code
>> address@hidden address@hidden
>> address@hidden -a @var{repo}
>> address@hidden identifies the archive repository from which to retrieve the
>> +information.  Currently the supported repositories and their identifiers
>> +are:
>> address@hidden -
>> address@hidden
>> address@hidden"http://elpa.gnu.org/packages";, GNU}, selected by the 
>> @code{gnu}
>> +identifier.  This is the default.
>> +
>> address@hidden
>> address@hidden"http://stable.melpa.org/packages";, MELPA-Stable}, selected by 
>> the
>> address@hidden identifier.
>> +
>> address@hidden
>> address@hidden"http://melpa.org/packages";, MELPA}, selected by the 
>> @code{melpa}
>> +identifier.
>> address@hidden itemize
>
> Perhaps REPO could also be a URL, in addition to one of these identifiers?

Yeah, perhaps later.

>> +;; Fetch URL, store the content in a temporary file and call PROC with that
>> +;; file.
>> +(define fetch-and-call-with-input-file
>> +  (memoize
>> +   (lambda* (url proc #:optional (err-msg "unavailable"))
>> +     (call-with-temporary-output-file
>> +      (lambda (temp port)
>> +        (or (and (url-fetch url temp)
>> +                 (call-with-input-file temp proc))
>> +            err-msg))))))
>
> Make the comment a docstring below ‘lambda*’.
>
> I would call it ‘call-with-downloaded-file’ for instance.  But then
> memoization should be moved elsewhere, because that’s not one would
> expect from a procedure with this name.

I've deleted memoization because it's not really helping in any way
here. It was helpful for experimenting in the REPL...

>> +(define (package-home-page alist)
>> +  "Extract the package home-page from ALIST."
>> +  (or (lookup-extra alist ':url) "unspecified"))
>
> Maybe use the ‘elpa-package’ prefix instead of ‘package’ for procedures
> like this one that expect an ELPA-package-as-an-alist?

'elpa-package-home-page' is already taken by a field accessor of the
record <elpa-package>. So, I've left it alone.

Thanks,
Fede

0001-import-Add-elpa-importer.patch
Description: Text Data