Re: [ELPA] A Setup package

[Stefan Monnier]

Hi Philip,

Philip K. [2021-02-04 12:43:11] wrote:
> I would like to propose adding a package to ELPA:

Have you already signed the copyright paperwork?
I see a possible matching "Philip K..." in the list, but its email is at
warpmail rather than posteo.  Is that you?

> ;;; setup.el --- Helpful Configuration Macro    -*- lexical-binding: t -*-
>
> ;; Author: Philip K. <philipk@posteo.net>
> ;; Maintainer: Philip K. <philipk@posteo.net>
> ;; Version: 0.1.0
> ;; Package-Requires: ((emacs "25.1"))
> ;; Keywords: lisp, local

Is this file available in a Git repository somewhere?

> ;;; Commentary:
>
> ;; The `setup' macro simplifies repetitive configuration patterns.
> ;; For example, these macros:
>
> ;;     (setup shell
> ;;       (let ((key "C-c s"))
> ;;         (global (key shell))
> ;;         (bind (key bury-buffer))))
> ;;
> ;;
> ;;     (setup (package paredit)
> ;;       (hide-lighter)
> ;;       (hook-into scheme-mode lisp-mode))

Ah, so it's to `use-package` a bit like `iterate` is to `loop`?
I like that.

> (eval-when-compile (require 'cl-macs))

Please use (require 'cl-lib) rather than (require 'cl-macs) because the
division of labor between those files is an internal detail.

> (defun setup-after-load (body)
>   "Wrap BODY in a `with-eval-after-load'."
>   ``(with-eval-after-load setup-name ,,body))

Hmm... that's a fairly subtle semantics.

> ;;;###autoload
> (defmacro setup (&rest body)
>   "Configure a feature or subsystem.
> BODY may contain special forms defined by `setup-define', but
> will otherwise just be evaluated as is."
>   (declare (indent defun))
>   (let* ((name (if (and (listp (car body))
>                         (get (caar body) 'setup-get-name))
>                    (funcall (get (caar body) 'setup-get-name)
>                             (car body))
>                  (car body)))

Hmm.. why do you use (&rest body) instead of (name &rest body)?

AFAICT this `setup-get-name` is so that you can say

    (setup (require shell)
      ...)

which like (setup shell ...) except that it `require`s shell eagerly.
And similarly

    (setup (package ivy)
      ...)

will try and install `ivy` if it's not installed yet.
Is it worth this complexity?  It seems you could get similar result
without this trick using a syntax like:

    (setup ivy
     (:get-package)
     (:require)
     ...)

which would save you this `setup-get-name` here and the corresponding
`:name` in `setup-define`.

> ;;;###autoload
> (defun setup-define (name fn &rest opts)
>   "Define `setup'-local macro NAME using function FN.
> The plist OPTS may contain the key-value pairs:
>
>   :name
> Specify a function to use, for extracting the feature name of a
> NAME entry, if it is the first element in a setup macro.
>
>   :indent
> Set the symbol property `lisp-indent-function' for NAME.
>
>   :wrap
> Specify a function used to wrap the results of a NAME entry.
>
>   :sig
> Give an advertised calling convention.
>
>   :doc
> A documentation string."

Here are some of the problem I see:
- You should add support for Edebug!
- `:indent` will set the `lisp-indent-function` property on the symbol,
  making it apply globally rather than only within `setup`.
  [ The same problem will plague the `:sig` and the Edebug support, BTW.  ]
  This is the reason why in `define-inline` I decided to prefix all the
  local macros with `inline-` :-(
  I don't have a really good solution for that (clearly, we'd like to
  allow `lisp-indent-function` (and Edebug's equivalent) to take some
  context into account, but that's not currently available.
  Patches welcome).
  But maybe to reduce the problem, you could use local macros with names
  that start with a `:`, as in:

            (setup shell
              (let ((key "C-c s"))
                (:global (key shell))
                (:bind (key bury-buffer))))

- I'm not sure the generality of `:wrap` is worthwhile, since it seems
  it's only ever used for eval-after-load.

>   ;; define macro for `cl-macrolet'
>   (push (let ((arity (func-arity fn)))
>           (cl-flet ((wrap (result)
>                           (if (plist-get opts :wrap)
>                               (funcall (plist-get opts :wrap) result)
>                             result)))
>             (cond ((eq (cdr arity) 'many)
>                    `(,name (&rest args) ,(wrap `(apply #',fn args))))
>                   ((eq (cdr arity) 0)
>                    `(,name () ,(wrap `(funcall #',fn))))
>                   ((= (car arity) (cdr arity))
>                    `(,name (&rest args)
>                            (unless (zerop (mod (length args) ,(car arity)))
>                              (error "Illegal arguments"))
>                            (let ((aggr (list 'progn)))
>                              (while args
>                                (let ((rest (nthcdr ,(car arity) args)))
>                                  (setf (nthcdr ,(car arity) args) nil)
>                                  (push (apply #',fn args) aggr)
>                                  (setq args rest)))
>                              ,(wrap `(nreverse aggr)))))
>                   ((error "Illegal function")))))
>         setup-macros)

IIUC this expands calls that have "more args" into repeated calls, right?
I suggest you make that behavior optional via some `:repeatable` keyword
argument, which will be an opportunity to document that feature.
That will also make it possible to accept `fn` values that allow optional
arguments.

> (defun setup-help ()
>   "Generate and display a help buffer for the `setup' macro."
>   (interactive)
>   (with-help-window (help-buffer)
>     (princ "The `setup' macro defines the following local macros:\n\n")
>     (dolist (sym (mapcar #'car setup-macros))
>       (let ((doc (or (get sym 'setup-documentation)
>                      "No documentation."))
>             (sig (if (get sym 'setup-signature)
>                      (cons sym (get sym 'setup-signature))
>                    (list sym))))
>         (princ (format "- %s\n\n%s\n\n" sig doc))))))

Take a look at the definition of `pcase` to see how you can merge the
above right into the normal function's doc, so you don't need a separate
command and `C-h o setup` will directly give you that info.

> (setup-define 'with-mode
>   (lambda (mode &rest body)
>     `(let ((setup-mode ',mode)
>            (setup-map ',(intern (format "%s-map" mode)))
>            (setup-hook ',(intern (format "%s-hook" mode))))
>        (ignore setup-mode setup-map setup-hook)
>        ,@body))
>   :sig '(MODE &body BODY)
>   :doc "Change the MODE that BODY is configuring."
>   :indent 1)

It would be nice to use this macro in the `setup` macro, to reduce the
code duplication between the two.

> (setup-define 'global
>   (lambda (bind)
>     (let ((key (car bind))
>           (fn (cadr bind)))
>       `(global-set-key ,(if (atom key) `(kbd ,key) key) #',fn)))

`atom`?  I think you only want to use `kbd` is `key` is a string, and
not if it's a vector, for example.

I find the syntax

    (bind (key bury-buffer))

a bit confusing from a Lisp point of view.  It would be nice to make it
into a *function* (or at least use the syntax that corresponds to
a function) so there's no confusion with a call to function `key`:

    (bind key 'bury-buffer)

An alternative would be to go the other way and use

    (bind ((key bury-buffer))

BTW, I think it'd be good to allow `setup` to have not only local macros
but also local functions (I think we should generally refrain from using
macros when functions work about as well; it simplifies the overall
system, leads to simpler semantics, and helps when debugging).

> (setup-define 'hide-lighter
>   (lambda ()
>     `(delq (assq setup-mode minor-mode-alist)
>            minor-mode-alist))
>   :doc "Hide the mode-line lighter of the current mode."
>   :body 'after-load)

I don't see `:body` handled anywhere.

> (setup-define 'local-set
>   (lambda (assign)
>     (let ((var (car assign))
>           (val (cadr assign)))
>       `(add-hook setup-hook (lambda () (setq-local ,var ,val)))))
>   :sig '((VAR VAL) *)
>   :doc "Set the value of VAR to VAL in buffers of the current mode."
>   :wrap #'setup-after-load)

Why after-load?


        Stefan