Alex Kost <alez...@gmail.com> skribis:
> From d42829fe03271e633e43cc35cf277705203e6080 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alez...@gmail.com>
> 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 ‘set!’ and hash tables, though. ;-)
Overall it looks OK. I’m concerned with code duplication between emacs/
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’d make it ‘manifest-entries->hash-table entries’.
> +(define manifest->hash-table
> + (let ((current-manifest #f)
> + (current-table #f))
> + (lambda (manifest)
> + "Return hash table of name keys and lists of matching MANIFEST
> entries."
> + (unless (manifest=? 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 mentry)))
> + (or (not output)
> + (equal? output (manifest-entry-output mentry)))))
> + mentries)
> + mentries)))
What about using ‘manifest-lookup’ instead?
> +(define (mentry-by-output mentries output)
> + (find (lambda (mentry)
> + (string= output (manifest-entry-output mentry)))
> + mentries))
Likewise.
> (define* (object-transformer param-alist #:optional (params '()))
> - "Return function for transforming an object into alist of
> parameters/values.
> + "Return function for transforming objects into alist of parameters/values.
“Return a procedure transforming an object into an list of
parameter/value pairs.”
> - (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
‘alist->record’.
> +(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 “package pattern”, right? I find 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’m unclear why this “package pattern” abstraction is needed
here.
Actually, is it just for serialization between Guile and Emacs? If that
is the case, then ‘package->sexp’ would seem more appropriate.
> +(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 ‘get’. The docstring is too terse.
> +;;; XXX move to (guix profiles) ?
> (define (profile-generations profile)
Definitely worth moving there (in a separate patch.)
Thanks,
Ludo’.
