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

[Alex Kost]

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