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

[Federico Beffa]

Oops, I've noticed that an unsupported repo was not handled gracefully.

On Thu, Jun 25, 2015 at 8:33 PM, Federico Beffa <address@hidden> wrote:
> 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