From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH 1/5] import: Add 'elpa' importer Date: Wed, 24 Jun 2015 23:06:43 +0200 Message-ID: <87381gvmsc.fsf@gnu.org> References: <87wpyw6bjl.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:57192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7rsi-0005Km-50 for guix-devel@gnu.org; Wed, 24 Jun 2015 17:06:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7rse-0006r7-Qy for guix-devel@gnu.org; Wed, 24 Jun 2015 17:06:52 -0400 In-Reply-To: (Federico Beffa's message of "Wed, 24 Jun 2015 18:15:12 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Federico Beffa Cc: Guix-devel , Alex Kost Federico Beffa skribis: > From 595befd0fb88ae00d405a727efb55b9fa456e549 Mon Sep 17 00:00:00 2001 > From: Federico Beffa > 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 i= n 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=3D@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 s= tring > +names." What about: Convert DEPS, a list of foobars =C3=A0 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=E2=80=99s best to give the predicate a name, and to n= ot define a =E2=80=98filter-xxx=E2=80=99 or =E2=80=98remove-xxx=E2=80=99 funct= ion. 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 t= hat > +;; 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 =E2=80=98lambda*=E2=80=99. I would call it =E2=80=98call-with-downloaded-file=E2=80=99 for instance. = But then memoization should be moved elsewhere, because that=E2=80=99s 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 =E2=80=9Cnormal=E2=80=9D return value, and because that=E2=80=99s a very un= usual 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 archi= eve 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 =E2=80=98match=E2=80=99. > +(define (lookup-extra alist key) > + "Lookup KEY from the ALIST extra package information." > + (assq-ref alist key)) Use =E2=80=98assq-ref=E2=80=99 directly. > +(define (package-home-page alist) > + "Extract the package home-page from ALIST." > + (or (lookup-extra alist ':url) "unspecified")) Maybe use the =E2=80=98elpa-package=E2=80=99 prefix instead of =E2=80=98pac= kage=E2=80=99 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 =E2=80=98ensure-list=E2=80=99. Thank you! Ludo=E2=80=99.