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

[Federico Beffa]

Thanks.

On Sun, Jun 21, 2015 at 10:39 PM, Alex Kost <address@hidden> wrote:
> Hi, I've tried this elpa importer and it is great!!
>
> I don't have real comments on code, just some nitpicks.
>
> Federico Beffa (2015-06-21 11:28 +0300) wrote:
>
>> From 8796b32a1ff8d565c3eb9874cde6a4a14d0b4f3b Mon Sep 17 00:00:00 2001
>> From: Federico Beffa <address@hidden>
>> Date: Tue, 16 Jun 2015 10:50:06 +0200
>> Subject: [PATCH 1/5] import: Add 'elpa' importer.
>>
>> * guix/import/elpa.scm: New file.
>> * guix/scripts/import.scm: Add "elpa" to 'importers'.
>> * guix/scripts/import/elpa.scm: New file.
>> * Makefile.am (MODULES): Add 'guix/import/elpa.scm' and
>>   'guix/scripts/import/elpa.scm'. (SCM_TESTS): Add 'tests/elpa.scm'.
>
> AFAIK the convention is to put "(...): ..." on a separate line:
>
>   * Makefile.am (MODULES): Add 'guix/import/elpa.scm' and
>     'guix/scripts/import/elpa.scm'.
>     (SCM_TESTS): Add 'tests/elpa.scm'.
>
> [...]
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index 46dccb8..3ca105a 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -3770,6 +3770,33 @@ package name by a hyphen and a version number as in 
>> the following example:
>>  @example
>>  guix import hackage mtl-2.1.3.1
>>  @end example
>> +
>> address@hidden elpa
>> address@hidden elpa
>> +Import meta-data from an Emacs ELPA package repository.
>> +
>> +Specific command-line options are:
>> +
>> address@hidden @code
>> address@hidden address@hidden
>> address@hidden -a @var{repo}
>> address@hidden identifies the archive repository to from which to retrive
>
> Redundant "to" (before "from")?  And a typo in "retrieve".
>
> [...]
>> +(define* (elpa-package->sexp pkg)
>
> There are some trailing spaces in this procedure:
>
>> +  "Return the `package' S-expression for the Emacs package PKG, a record of
>> +type '<elpa-package>'."
>> +
> here^
>
>> +  (define name (elpa-package-name pkg))
>> +
> here^
>
>> +  (define version (elpa-package-version pkg))
>> +
> here^
>
>> +  (define source-url (elpa-package-source-url pkg))
>> +
> here^
>
>> +  (define dependencies
>> +    (let* ((deps (elpa-package-inputs pkg))
>> +           (names (filter-dependencies (elpa-dependencies->names deps))))
>> +      (map (lambda (n)
>> +             (let ((new-n (elpa-name->package-name n)))
>> +               (list new-n (list 'unquote (string->symbol new-n)))))
>> +           names)))
>> +
> here^
>
>> +  (define (maybe-inputs input-type inputs)
>> +    (match inputs
>> +      (()
>> +       '())
>> +      ((inputs ...)
>> +       (list (list input-type
>> +                   (list 'quasiquote inputs))))))
>> +
> here^
>
> [...]
>> +;;; Command-line options.
>> +;;;
>> +
>> +(define %default-options
>> +  '((repo . "gnu")))
>> +
>> +(define (show-help)
>> +  (display (_ "Usage: guix import elpa PACKAGE-NAME
>> +Import the latest package named PACKAGE-NAME from an ELPA repository.\n"))
>> +  (display (_ "
>> +  -a ARCHIVE, --archive=ARCHIVE  specify the archive repository"))
>
> I think we don't duplicate an option argument (see "guix package --help"
> for example):
>
>   -a, --archive=ARCHIVE  specify the archive repository
>
> --
> Alex

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