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

[ludo@gnu.org (Ludovic Courtès)]

Federico Beffa <beffa@ieee.org> skribis:

> From 595befd0fb88ae00d405a727efb55b9fa456e549 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <beffa@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’.