[bug#28832] [PATCH 1/3] gnu: Add emacs-json-reformat.

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

Hi Oleg,

I think it would have been better to open a separate issue for this.
Here are some superficial comments.  I hope someone more knowledgeable
about Emacs can comment.

Oleg Pykhalov <go.wigust@gmail.com> skribis:

> From adda59022a61dfcf2add1d9f01d02df4be90a0f1 Mon Sep 17 00:00:00 2001
> From: Oleg Pykhalov <go.wigust@gmail.com>
> Date: Wed, 13 Dec 2017 08:10:21 +0300
> Subject: [PATCH] emacs-build-system: Add EMACSLOADPATH.
>
> * guix/build/emacs-build-system.scm: Add EMACSLOADPATH.

Please see ‘C-x v l’ in that file for the syntax of commit logs.  :-)

> +(define (store-directory->package-name store-dir)
> +  "Extract package name from STORE-DIR."
> +  (let-values (((name _) ((compose package-name->name+version
> +                                   strip-store-file-name)
> +                          store-dir)))
> +    name))

It’s enough to write it like this:

  (define store-directory->package-name
    (compose package-name->name+version
             strip-store-file-name))

Guile does automatic “multiple-value truncation”, which means that the
second value that the procedure returns can be ignored by the caller.

> +(define (store-directory->package-version store-dir)
> +  "Extract package version from STORE-DIR."
> +  (let-values (((_ version) ((compose package-name->name+version
> +                                      strip-store-file-name)
> +                             store-dir)))
> +    version))

Likewise.

> +(define* (setup-environment #:key inputs #:allow-other-keys)
> +  "Export the variable EMACSLOADPATH, which are based on INPUTS respectively."
> +  (let* ((filtered-inputs (emacs-inputs inputs))
> +         (emacs-input-dir (cdr (assoc "emacs" filtered-inputs)))
> +         (inputs-dirs (cdr filtered-inputs)))

Nitpick: Please see the guidelines mentioned in
<https://www.gnu.org/software/guix/manual/html_node/Formatting-Code.html>
on how to choose identifiers.  I’d write:

  (let* ((inputs (emacs-inputs inputs))
         (emacs  (assoc-ref inputs "emacs")))
    …)

‘inputs-dirs’ is unneeded AIUI (see below).

> +    ;; EMACSLOADPATH is where Emacs looks for the source code of the build's
> +    ;; dependencies.
> +    (setenv "EMACSLOADPATH"
> +            (string-append emacs-input-dir "/share/emacs/"
> +                           (store-directory->package-version emacs-input-dir)
> +                           "/lisp"))
> +    (for-each
> +     (lambda (input)
> +       (let ((store-item (cdr input)))

Rather:

  (for-each (match-lambda
              ((name . input)
               …))
            …)

> +         (setenv "EMACSLOADPATH"
> +                 (string-append
> +                  (or (getenv "EMACSLOADPATH") "")
> +                  ":" store-item %install-suffix "/"
> +                  ((compose string-drop-emacs store-directory->package-name)
> +                   store-item)))))

Rather:

  (string-drop-emacs (store-directory->package-name item))

IMO ‘compose’ makes things less readable in this case.

> +     ((compose (lambda (inputs) (alist-delete "emacs" inputs))
> +               (lambda (inputs) (alist-delete "source" inputs)))
> +      (delete-duplicates inputs-dirs)))

Rather:

  (fold alist-delete inputs '("emacs" "source"))

Did you try rebuilding Emacs packages, and to simplify those that
explicitly pass -L flags?

Thanks a lot for working on it!

Ludo’.