[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/5] import: Add 'elpa' importer
From: |
Federico Beffa |
Subject: |
Re: [PATCH 1/5] import: Add 'elpa' importer |
Date: |
Thu, 25 Jun 2015 20:33:05 +0200 |
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