[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/5] import: Add 'elpa' importer
From: |
Alex Kost |
Subject: |
Re: [PATCH 1/5] import: Add 'elpa' importer |
Date: |
Sun, 21 Jun 2015 23:39:42 +0300 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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