Federico Beffa <be...@ieee.org> skribis:
> From 595befd0fb88ae00d405a727efb55b9fa456e549 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <be...@fbengineering.ch>
> 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'.
> * doc/guix.texi (Invoking guix import): Document it.
> * tests/elpa.scm: New file.
This looks nice! (And you were fast!)
Below are some comments essentially nitpicking about the style.
> --- 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
> +
> +@item elpa
> +@cindex elpa
> +Import meta-data from an Emacs ELPA package repository.
Rather:
from an Emacs Lisp Package Archive (ELPA) package repository
(@pxref{Packages,,, emacs, The GNU Emacs Manual}).
> +Specific command-line options are:
> +
> +@table @code
> +@item --archive=@var{repo}
> +@itemx -a @var{repo}
> +@var{repo} identifies the archive repository from which to retrieve the
> +information. Currently the supported repositories and their identifiers
> +are:
> +@itemize -
> +@item
> +@uref{"http://elpa.gnu.org/packages";, GNU}, selected by the @code{gnu}
> +identifier. This is the default.
> +
> +@item
> +@uref{"http://stable.melpa.org/packages";, MELPA-Stable}, selected by the
> +@code{melpa-stable} identifier.
> +
> +@item
> +@uref{"http://melpa.org/packages";, MELPA}, selected by the @code{melpa}
> +identifier.
> +@end itemize
Perhaps REPO could also be a URL, in addition to one of these identifiers?
> +(define (elpa-dependencies->names deps)
> + "Convert the list of dependencies from the ELPA format, to a list of string
> +names."
What about:
Convert DEPS, a list of foobars à la ELPA, to a list of package names
as strings.
> + (map (lambda (d) (symbol->string (first d))) deps))
(match deps
(((names _ ...) ...)
(map symbol->string names)))
> +(define (filter-dependencies names)
> + "Remove the package names included with Emacs from the list of
> +NAMES (strings)."
> + (let ((emacs-standard-libraries
> + '("emacs" "cl-lib")))
> + (filter (lambda (d) (not (member d emacs-standard-libraries)))
> + names)))
In general I think it’s best to give the predicate a name, and to not
define a ‘filter-xxx’ or ‘remove-xxx’ function. So:
(define emacs-standard-library?
(let ((libs '("emacs" "cl-lib")))
(lambda (lib)
"Return true if ..."
(member lib libs))))
and then:
(filter emacs-standard-library? names)
wherever needed.
> +(define* (elpa-url #:optional (repo "gnu"))
I would rather use a symbol for REPO.
> +;; 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.
The return value must also be documented. Returning an error message is
not an option IMO, because the caller could hardly distinguish it from a
“normal” return value, and because that’s a very unusual convention.
Probably an error condition should be raised?
> +(define* (elpa-package-info name #:optional (repo "gnu"))
> + "Extract the information about the package NAME from the package archieve
> of
> +REPO."
> + (let* ((archive (elpa-fetch-archive repo))
> + (info (filter (lambda (p) (eq? (first p) (string->symbol name)))
> + (cdr archive))))
> + (if (pair? info) (first info) #f)))
Give the predicate a name, and rather use ‘match’.
> +(define (lookup-extra alist key)
> + "Lookup KEY from the ALIST extra package information."
> + (assq-ref alist key))
Use ‘assq-ref’ directly.
> +(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?
> +(define (nil->empty alist)
> + "If ALIST is the symbol 'nil return the empty list. Otherwise, return
> ALIST."
Rather ‘ensure-list’.
Thank you!
Ludo’.
