From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] emacs: Rewrite scheme side in a functional manner. Date: Sat, 20 Sep 2014 16:11:41 +0200 Message-ID: <87oaua5qle.fsf_-_@gnu.org> References: <87a96e7bu3.fsf@gmail.com> <87egvq1nj2.fsf@gnu.org> <87y4ty5jl9.fsf@gmail.com> <8738c5vmuv.fsf@gnu.org> <87lhpw66kq.fsf@gmail.com> <87y4twek0u.fsf@taylan.uni.cx> <87bnqssbc0.fsf_-_@gnu.org> <87sijodrl2.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]:50468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVLOJ-0000Ry-G0 for guix-devel@gnu.org; Sat, 20 Sep 2014 10:12:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XVLOA-0006NK-42 for guix-devel@gnu.org; Sat, 20 Sep 2014 10:11:59 -0400 Received: from hera.aquilenet.fr ([2a01:474::1]:51876) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVLO9-0006Mr-L0 for guix-devel@gnu.org; Sat, 20 Sep 2014 10:11:50 -0400 In-Reply-To: <87sijodrl2.fsf@gmail.com> (Alex Kost's message of "Fri, 19 Sep 2014 10:58:33 +0400") 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: Alex Kost Cc: guix-devel@gnu.org Alex Kost skribis: > From d42829fe03271e633e43cc35cf277705203e6080 Mon Sep 17 00:00:00 2001 > From: Alex Kost > Date: Thu, 18 Sep 2014 16:24:02 +0400 > Subject: [PATCH 2/3] emacs: Rewrite scheme side in a functional manner. Good idea! There are still a bunch of =E2=80=98set!=E2=80=99 and hash tables, though. = ;-) Overall it looks OK. I=E2=80=99m concerned with code duplication between e= macs/ and guix/, though, and there are also a few stylistic issues, and missing or terse docstrings. Some remarks: > +(define (mentries->hash-table mentries) For consistency I=E2=80=99d make it =E2=80=98manifest-entries->hash-table e= ntries=E2=80=99. > +(define manifest->hash-table > + (let ((current-manifest #f) > + (current-table #f)) > + (lambda (manifest) > + "Return hash table of name keys and lists of matching MANIFEST ent= ries." > + (unless (manifest=3D? manifest current-manifest) > + (set! current-manifest manifest) > + (set! current-table (mentries->hash-table > + (manifest-entries manifest)))) > + current-table))) What about: (define manifest->hash-table (memoize (compose manifest-entries->hash-table manifest-entries))) But honestly I think this is premature optimization (I mean, there are 177 packages in my profile, so 10,000 ;-)), and should that optimization be needed, it should be done transparently in (guix profiles), as we discussed some time ago. > +(define* (mentries-by-name manifest name #:optional version output) > + "Return list of MANIFEST entries matching NAME, VERSION and OUTPUT." > + (let ((mentries (or (hash-ref (manifest->hash-table manifest) name) > + '()))) > + (if (or version output) > + (filter (lambda (mentry) > + (and (or (not version) > + (equal? version (manifest-entry-version mentr= y))) > + (or (not output) > + (equal? output (manifest-entry-output mentry= ))))) > + mentries) > + mentries))) What about using =E2=80=98manifest-lookup=E2=80=99 instead? > +(define (mentry-by-output mentries output) > + (find (lambda (mentry) > + (string=3D output (manifest-entry-output mentry))) > + mentries)) Likewise. > (define* (object-transformer param-alist #:optional (params '())) > - "Return function for transforming an object into alist of parameters/v= alues. > + "Return function for transforming objects into alist of parameters/val= ues. =E2=80=9CReturn a procedure transforming an object into an list of parameter/value pairs.=E2=80=9D > - (lambda (object) > + (lambda objects > (map (match-lambda > ((param . fun) > - (cons param (fun object)))) > + (cons param (apply fun objects)))) > alist)))) s/fun/proc/ (yeah, this is Scheme. ;-)) May be worth considering moving it to (guix records), which already has =E2=80=98alist->record=E2=80=99. > +(define (spec->package-pattern spec) > + (call-with-values > + (lambda () (full-name->name+version spec)) > + list)) s/spec/specification/g However, the result is not a =E2=80=9Cpackage pattern=E2=80=9D, right? I f= ind the name a bit confusing. Is there any reason to use lists instead of multiple values? > +(define (manifest-package-patterns manifest) > + "Return list of package patterns for all MANIFEST entries." > + (fold-manifest-by-name manifest > + (lambda (name version mentries res) > + (cons (list name version mentries) res)) > + '())) Likewise I=E2=80=99m unclear why this =E2=80=9Cpackage pattern=E2=80=9D abs= traction is needed here. Actually, is it just for serialization between Guile and Emacs? If that is the case, then =E2=80=98package->sexp=E2=80=99 would seem more appropria= te. > +(define (package-pattern-transformer manifest params) > + "Return 'package-pattern->package-entries' function." Damn, what does this mean? :-) > +(define (get-package/output-entries profile params entry-type > + search-type search-vals) > + "Return list of package or output entries." Never =E2=80=98get=E2=80=99. The docstring is too terse. > +;;; XXX move to (guix profiles) ? > (define (profile-generations profile) Definitely worth moving there (in a separate patch.) Thanks, Ludo=E2=80=99.