Re: [ELPA] A Setup package

[Stefan Monnier]

>> We don't yet have http://elpa.gnu.org/packages/setup.html OTOH because
>> the 0.1.0 version of the code is missing the copyright changes (the
>> release is made from the commit in which you bump the version number).
>> So if you want to make a release based on this code, you'll want to bump
>> up the version number.
> What exactly is missing?

You missed this part:

    (the release is made from the commit in which you bump the version
     number)

the commit in which you changed `Version:` to the current 0.1.0 did not
have a "copyright FSF" line.

> My understanding was that I had to update the
> header and the COPYING file.

Actually, we don't include the COPYING file is most cases.  After all,
the GPLv3 license comes with Emacs, so the user obviously has
a copy already ;-)

>> Here's some other suggestions.
> I like most of the suggestions, would you mind me committing them with
> you as the author?

Fine by me.

>> Obviously the use of `help--make-usage` is problematic since the `--`
>> indicates it's not supposed to be used by "outsiders", so we should
>> change help.el or help-fns.el to provide a proper function for that
>> (pcase uses `help-fns--signature` instead, which suffers from the same
>> problem).
>
> Why use help--make-usage at all?

To avoid reinvent the wheel.

>>         (let (specs)
>>           (dolist (name (mapcar #'car setup-macros))
>>             (let ((body (cond ((eq (get name 'setup-debug) 'none) nil)
>>                               ((get name 'setup-debug) nil)
>>                               ('(sexp)))))
>> -             (push (if (get name 'setup-repeatable)
>> +             (push (if (plist-get opts :repeatable)
>
> This doesn't work! The specification is regenerated for every
> setup-define call,

Indeed, sorry 'bout that.

> It might be better to save the specification in a separate variable and
> modify this destructively for every setup-define call, so as to avoid
> the overhead of redefining the entire specification all the time.

I wouldn't bother, no.  In the patch below I instead did the
spec-processing when setting `setup-debug` (and I also dropped the
`none` support because I don't see what it gains: for a macro that
takes no arguments, `&rest sexp` works as well).
And there is a better option for Emacs-28 and after.


        Stefan


diff --git a/setup.el b/setup.el
index 510c599d5f..eca9f23507 100644
--- a/setup.el
+++ b/setup.el
@@ -88,10 +88,8 @@
     (insert (documentation (symbol-function 'setup) 'raw))
     (dolist (sym (sort (mapcar #'car setup-macros)
                        #'string-lessp))
-      (let ((sig (if (get sym 'setup-signature)
-                     (cons sym (get sym 'setup-signature))
-                   (list sym))))
-        (insert (format " - %s\n\n" sig)
+      (let ((sig (get sym 'setup-signature)))
+        (insert (format " - %s\n\n" (help--make-usage sym sig))
                 (or (get sym 'setup-documentation)
                     "No documentation.")
                 "\n\n")))
@@ -122,55 +120,59 @@ The following local macros are defined in a `setup' 
body:\n\n"
   "Define `setup'-local macro NAME using function FN.
 The plist OPTS may contain the key-value pairs:
 
-  :name
+  :name 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
+  :indent SPEC
 Change indentation behaviour.  See symbol `lisp-indent-function'.
 
-  :after-loaded
+  :after-loaded BOOL
 Wrap the macro in a `with-eval-after-load' body.
 
-  :repeatable
-Allow macro to be automatically repeated, using FN's arity.
+  :repeatable ARITY
+Allow macro to be automatically repeated.
 
-  :signature
+  :signature SIG
 Give an advertised calling convention.
 
-  :documentation
+  :documentation STRING
 A documentation string.
 
-  :debug
-A edebug specification, see Info node `(elisp)
-Specification List'.  If not given, it is assumed nothing is
-evaluated.  If macro is :repeatable, a &rest will be added before
-the specification."
+  :debug SPEC
+A edebug specification, see Info node `(elisp) Specification List'.
+If not given, it is assumed nothing is evaluated."
   (declare (indent 1))
   (cl-assert (symbolp name))
   (cl-assert (functionp fn))
   (cl-assert (listp opts))
   ;; save metadata
   (put name 'setup-documentation (plist-get opts :documentation))
-  (put name 'setup-signature (plist-get opts :signature))
+  (put name 'setup-signature
+       (or (plist-get opts :signature)
+           (append (help-function-arglist fn 'preserve-names)
+                   (if (plist-get opts :repeatable) '(...)))))
   (put name 'setup-shorthand (plist-get opts :shorthand))
   (put name 'lisp-indent-function (plist-get opts :indent))
   (put name 'setup-indent (plist-get opts :indent))
-  (put name 'setup-repeatable (plist-get opts :repeatable))
-  (put name 'setup-debug (plist-get opts :debug))
+  ;; (put name 'setup-repeatable (plist-get opts :repeatable))
+  (put name 'setup-debug
+       (let ((spec (plist-get opts :debug)))
+         (if (plist-get opts :repeatable)
+             (cons '&rest spec) spec)))
   ;; forget previous definition
   (setq setup-macros (delq (assq name setup-macros)
                            setup-macros))
   ;; define macro for `cl-macrolet'
-  (push (let* ((arity (func-arity fn))
-               (body (if (plist-get opts :repeatable)
+  (push (let* ((arity (plist-get opts :repeatable))
+               (body (if arity
                          `(progn
-                            (unless (zerop (mod (length args) ,(car arity)))
+                            (unless (zerop (mod (length args) ,arity))
                               (error "Illegal arguments"))
                             (let (aggr)
                               (while args
-                                (let ((rest (nthcdr ,(car arity) args)))
-                                  (setf (nthcdr ,(car arity) args) nil)
+                                (let ((rest (nthcdr ,arity args)))
+                                  (setf (nthcdr ,arity args) nil)
                                   (push (apply #',fn args) aggr)
                                   (setq args rest)))
                               `(progn ,@(nreverse aggr))))
@@ -180,16 +182,13 @@ the specification."
                       `(with-eval-after-load setup-name ,,body))
             `(,name (&rest args) `,,body)))
         setup-macros)
+  ;; FIXME: Use `&interpose' in Emacs≥28.
   (put 'setup 'edebug-form-spec
        (let (specs)
          (dolist (name (mapcar #'car setup-macros))
-           (let ((body (cond ((eq (get name 'setup-debug) 'none) nil)
-                             ((get name 'setup-debug) nil)
-                             ('(sexp)))))
-             (push (if (get name 'setup-repeatable)
-                       `(,(symbol-name name) &rest ,@body)
-                     `(,(symbol-name name) ,@body))
-                   specs)))
+           (push `(,(symbol-name name)
+                   ,@(or (get name 'setup-debug) '(&rest sexp)))
+                 specs))
          `(&rest &or [symbolp sexp] ,@specs form))))
 

@@ -215,7 +214,7 @@ the specification."
            (setup-hook ',(intern (format "%s-hook" mode))))
        (ignore setup-mode setup-map setup-hook)
        ,@body))
-  :signature '(MODE &body BODY)
+  ;; :signature '(MODE &body BODY)
   :documentation "Change the MODE that BODY is configuring."
   :debug '(sexp setup)
   :indent 1)
@@ -224,7 +223,7 @@ the specification."
   (lambda (map &rest body)
     `(let ((setup-map ',map))
        ,@body))
-  :signature '(MAP &body BODY)
+  ;; :signature '(MAP &body BODY)
   :documentation "Change the MAP that BODY will bind to"
   :debug '(sexp setup)
   :indent 1)
@@ -233,7 +232,7 @@ the specification."
   (lambda (hook &rest body)
     `(let ((setup-hook ',hook))
        ,@body))
-  :signature '(HOOK &body BODY)
+  ;; :signature '(HOOK &body BODY)
   :documentation "Change the HOOK that BODY will use."
   :debug '(sexp setup)
   :indent 1)
@@ -242,18 +241,18 @@ the specification."
   (lambda (package)
     `(unless (package-installed-p ',package)
        (package-install ',package)))
-  :signature '(PACKAGE ...)
+  ;; :signature '(PACKAGE ...)
   :documentation "Install PACKAGE if it hasn't been installed yet."
   :shorthand #'cadr
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :require
   (lambda (feature)
     `(require ',feature))
-  :signature '(FEATURE ...)
+  ;; :signature '(FEATURE ...)
   :documentation "Eagerly require FEATURE."
   :shorthand #'cadr
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :global
   (lambda (key fn)
@@ -265,7 +264,7 @@ the specification."
   :signature '(KEY FUNCTION ...)
   :documentation "Globally bind KEY to FUNCTION."
   :debug '(form [&or [symbolp sexp] form])
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :bind
   (lambda (key fn)
@@ -278,7 +277,7 @@ the specification."
   :documentation "Bind KEY to FUNCTION in current map."
   :after-loaded t
   :debug '(form [&or [symbolp sexp] form])
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :unbind
   (lambda (key)
@@ -287,11 +286,11 @@ the specification."
               `(kbd ,key)
           ,key)
        nil))
-  :signature '(KEY ...)
+  ;; :signature '(KEY ...)
   :documentation "Unbind KEY in current map."
   :after-loaded t
   :debug '(form)
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :rebind
   (lambda (key fn)
@@ -306,23 +305,23 @@ the specification."
   :signature '(KEY FUNCTION ...)
   :documentation "Unbind the current key for FUNCTION, and bind it to KEY."
   :after-loaded t
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :hook
-  (lambda (hook)
-    `(add-hook setup-hook #',hook))
-  :signature '(FUNCTION ...)
+  (lambda (function)
+    `(add-hook setup-hook #',function))
+  ;; :signature '(FUNCTION ...)
   :documentation "Add FUNCTION to current hook."
   :debug '(form [&or [symbolp sexp] form])
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :hook-into
   (lambda (mode)
     `(add-hook ',(intern (concat (symbol-name mode) "-hook"))
                setup-mode))
-  :signature '(HOOK ...)
+  ;; :signature '(HOOK ...)
   :documentation "Add current mode to HOOK."
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :option
   (lambda (var val)
@@ -349,14 +348,14 @@ will use the car value to modify the behaviour.  If NAME 
has the
 form (append VAR), VAL is appended to VAR.  If NAME has the
 form (prepend VAR), VAL is prepended to VAR."
   :debug '(sexp form)
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :hide-mode
   (lambda ()
     `(delq (assq setup-mode minor-mode-alist)
            minor-mode-alist))
   :documentation "Hide the mode-line lighter of the current mode."
-  :debug 'none
+  ;; :debug 'none
   :after-loaded t)
 
 (setup-define :local-set
@@ -370,7 +369,7 @@ form (prepend VAR), VAL is prepended to VAR."
                  val `(cons ,val ,name)))
           ((error "Invalid variable %S" name)))
     `(add-hook setup-hook (lambda () (setq-local ,name ,val))))
-  :signature '(name VAL ...)
+  ;; :signature '(name VAL ...)
   :documentation "Set the value of NAME to VAL in buffers of the current mode.
 
 NAME may be a symbol, or a cons-cell.  If NAME is a cons-cell, it
@@ -378,7 +377,7 @@ will use the car value to modify the behaviour.  If NAME 
has the
 form (append VAR), VAL is appended to VAR.  If NAME has the
 form (prepend VAR), VAL is prepended to VAR."
   :debug '(sexp form)
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :local-hook
   (lambda (hook fn)
@@ -388,36 +387,38 @@ form (prepend VAR), VAL is prepended to VAR."
   :signature '(HOOK FUNCTION ...)
   :documentation "Add FUNCTION to HOOK only in buffers of the current mode."
   :debug '(symbolp form)
-  :repeatable t)
+  :repeatable 2)
 
 (setup-define :also-load
   (lambda (feature)
     `(require ',feature))
-  :signature '(FEATURE ...)
+  ;; :signature '(FEATURE ...)
   :documentation "Load FEATURE with the current body."
-  :repeatable t
+  :repeatable 1
   :after-loaded t)
 
 (setup-define :needs
-  (lambda (binary)
-    `(unless (executable-find ,binary)
+  (lambda (executable)
+    `(unless (executable-find ,executable)
        (throw 'setup-exit nil)))
-  :signature '(PROGRAM ...)
-  :documentation "If PROGRAM is not in the path, stop here."
-  :repeatable t)
+  ;; :signature '(PROGRAM ...)
+  :documentation "If EXECUTABLE is not in the path, stop here."
+  :repeatable 1)
 
 (setup-define :if
   (lambda (condition)
     `(unless ,condition
        (throw 'setup-exit nil)))
-  :signature '(CONDITION ...)
+  ;; :signature '(CONDITION ...)
+  ;; FIXME: I find this semantics odd for this name.  Maybe you should rename
+  ;; it to `:stop-if'?
   :documentation "If CONDITION is non-nil, stop evaluating the body."
   :debug '(form)
-  :repeatable t)
+  :repeatable 1)
 
 (setup-define :when-loaded
   (lambda (&rest body) `(progn ,@body))
-  :signature '(&body BODY)
+  ;; :signature '(&body BODY)
   :documentation "Evaluate BODY after the current feature has been loaded."
   :debug '(body)
   :after-loaded t)