Re: [PATCH] emacs: Rewrite scheme side in a functional manner.

[Ludovic Courtès]

Alex Kost <address@hidden> skribis:

> From d42829fe03271e633e43cc35cf277705203e6080 Mon Sep 17 00:00:00 2001
> From: Alex Kost <address@hidden>
> 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’.