~pkal/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 2

[PATCH] Generic implementation of setup-ensure-spec

Details
Message ID
<875yb150b8.fsf@treypeacock.com>
DKIM signature
missing
Download raw message
Patch: +31 -17
I currently use a modified versions of the keybinding macros that use
`keymap-set` and other functions found in `keymap.el`. This resulted in
an error thrown when trying to use the new `kbd` ensure keyword. "C-="
can be set with `keymap-set`, but it will result an error in
`setup--ensure` with `(kbd "C-=")`.

So, I figured it may be useful to allow for custom ENSURE-SPECs. The
implementation below uses `cl-defgeneric` to allow for custom methods;
however, if you do not want to rely on the `cl-lib` library, I have a
patch that uses an alist variable instead. This seemed a bit cleaner to
me.

For example, this is the spec that I use for by custom `:bind` macros:

(cl-defmethod setup-ensure-spec ((spec (eql key-valid)) arg)
  (if (key-valid-p arg)
      arg
    (setup-ensure-spec 'kbd arg)))

Do you think this is something you would be interested in?


---
 setup.el | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/setup.el b/setup.el
index 4574487..1d4f1de 100644
--- a/setup.el
+++ b/setup.el
@@ -43,6 +43,7 @@

;;; Code:

(require 'cl)
(require 'elisp-mode)

(defvar setup-opts `((quit . ,(make-symbol "setup-quit")))
@@ -144,11 +145,35 @@ (defmacro setup (name &rest body)
;;;###autoload
(put 'setup 'function-documentation '(setup--make-docstring))

(cl-defgeneric setup-ensure-spec (ensure-spec arg)
  "Ensure that ARGS matches the form of ENSURE-SPEC.
Return nil by default."
  nil)

(cl-defmethod setup-ensure-spec ((_ (eql nil)) arg)
  arg)

(cl-defmethod setup-ensure-spec ((_ (eql kbd)) arg)
  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
  (cond
   ((stringp arg) (kbd arg))
   ((symbolp arg) `(kbd ,arg))
   (arg)))

(cl-defmethod setup-ensure-spec ((_ (eql func)) arg)
  "Ensure ARG is a function, sharp-quoted if necessary."
  (cond
   ((eq (car-safe arg) 'function)
    arg)
   ((eq (car-safe arg) 'quote)
    `#',(cadr arg))
   ((symbolp arg)
    `#',arg)
   (arg)))

(defun setup--ensure (ensure-spec args &optional check-len)
  "Ensure that ARGS matches the form of ENSURE-SPEC.

The symbol `kbd' means to apply the function `kbd' to the
argument.  The symbol `func' means to sharp-quote the argument.
The symbol `&rest' means that the remaining elements of
ENSURE-SPEC are applied repeatedly to the remaining elements of
ARGS.
@@ -188,21 +213,10 @@ (defun setup--ensure (ensure-spec args &optional check-len)
                                        nil))
                  (push ensured result))))
          (let ((arg (pop args)))
            (push (cond ((null ensure) arg) ;Do not modify argument
                        ((eq ensure 'kbd) (cond
                                           ((stringp arg) (kbd arg))
                                           ((symbolp arg) `(kbd ,arg))
                                           (arg)))
                        ((eq ensure 'func) (cond
                                            ((eq (car-safe arg) 'function)
                                             arg)
                                            ((eq (car-safe arg) 'quote)
                                             `#',(cadr arg))
                                            ((symbolp arg)
                                             `#',arg)
                                            (arg)))
                        ((error "Invalid ensure spec %S" ensure)))
                  result)))))
            (push
             (or (setup-ensure-spec ensure arg)
                 (error "Invalid ensure spec %S" ensure))
             result)))))
    (nreverse result)))

(defun setup-define (name fn &rest opts)
--
2.40.0
Details
Message ID
<87v8j1love.fsf@posteo.net>
In-Reply-To
<875yb150b8.fsf@treypeacock.com> (view parent)
DKIM signature
missing
Download raw message
Hi,

Trey Peacock <git@treypeacock.com> writes:

> I currently use a modified versions of the keybinding macros that use
> `keymap-set` and other functions found in `keymap.el`. This resulted in
> an error thrown when trying to use the new `kbd` ensure keyword. "C-="
> can be set with `keymap-set`, but it will result an error in
> `setup--ensure` with `(kbd "C-=")`.
>
> So, I figured it may be useful to allow for custom ENSURE-SPECs. The
> implementation below uses `cl-defgeneric` to allow for custom methods;
> however, if you do not want to rely on the `cl-lib` library, I have a
> patch that uses an alist variable instead. This seemed a bit cleaner to
> me.
>
> For example, this is the spec that I use for by custom `:bind` macros:
>
> (cl-defmethod setup-ensure-spec ((spec (eql key-valid)) arg)
>   (if (key-valid-p arg)
>       arg
>     (setup-ensure-spec 'kbd arg)))
>
> Do you think this is something you would be interested in?

I get the impression that cl-defmacro is too great of an overhead here
-- especially when we consider loading the necessary files during
initialisation.  `Setup' is relatively fast, and if there is a more
efficient/lightweight way then I think it would be preferable.
Dispatching ensure-specs could also be done using an alist mapping
symbols to functions.  That being said, I wonder if this is really so
much potential for extending `setup--ensure' or if simply extending it
from time to time is the good enough of a solution?  Especially when we
keep in mind that for infrequent objects it is often just easier to
preform the check yourself in the local macro instead of using an
ensure-spec.

>
> ---
>  setup.el | 48 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/setup.el b/setup.el
> index 4574487..1d4f1de 100644
> --- a/setup.el
> +++ b/setup.el
> @@ -43,6 +43,7 @@
>
>  ;;; Code:
>
> +(require 'cl)

Watch out, you want to load cl-generic, not cl -- that is deprecated.

>  (require 'elisp-mode)
>
>  (defvar setup-opts `((quit . ,(make-symbol "setup-quit")))
> @@ -144,11 +145,35 @@ (defmacro setup (name &rest body)
>  ;;;###autoload
>  (put 'setup 'function-documentation '(setup--make-docstring))
>
> +(cl-defgeneric setup-ensure-spec (ensure-spec arg)
> +  "Ensure that ARGS matches the form of ENSURE-SPEC.
> +Return nil by default."
> +  nil)
> +
> +(cl-defmethod setup-ensure-spec ((_ (eql nil)) arg)
> +  arg)
> +
> +(cl-defmethod setup-ensure-spec ((_ (eql kbd)) arg)
> +  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
> +  (cond
> +   ((stringp arg) (kbd arg))
> +   ((symbolp arg) `(kbd ,arg))
> +   (arg)))
> +
> +(cl-defmethod setup-ensure-spec ((_ (eql func)) arg)
> +  "Ensure ARG is a function, sharp-quoted if necessary."
> +  (cond
> +   ((eq (car-safe arg) 'function)
> +    arg)
> +   ((eq (car-safe arg) 'quote)
> +    `#',(cadr arg))
> +   ((symbolp arg)
> +    `#',arg)
> +   (arg)))
> +
>  (defun setup--ensure (ensure-spec args &optional check-len)
>    "Ensure that ARGS matches the form of ENSURE-SPEC.
>
> -The symbol `kbd' means to apply the function `kbd' to the
> -argument.  The symbol `func' means to sharp-quote the argument.
>  The symbol `&rest' means that the remaining elements of
>  ENSURE-SPEC are applied repeatedly to the remaining elements of
>  ARGS.
> @@ -188,21 +213,10 @@ (defun setup--ensure (ensure-spec args &optional check-len)
>                                          nil))
>                    (push ensured result))))
>            (let ((arg (pop args)))
> -            (push (cond ((null ensure) arg) ;Do not modify argument
> -                        ((eq ensure 'kbd) (cond
> -                                           ((stringp arg) (kbd arg))
> -                                           ((symbolp arg) `(kbd ,arg))
> -                                           (arg)))
> -                        ((eq ensure 'func) (cond
> -                                            ((eq (car-safe arg) 'function)
> -                                             arg)
> -                                            ((eq (car-safe arg) 'quote)
> -                                             `#',(cadr arg))
> -                                            ((symbolp arg)
> -                                             `#',arg)
> -                                            (arg)))
> -                        ((error "Invalid ensure spec %S" ensure)))
> -                  result)))))
> +            (push
> +             (or (setup-ensure-spec ensure arg)
> +                 (error "Invalid ensure spec %S" ensure))
> +             result)))))
>      (nreverse result)))
>
>  (defun setup-define (name fn &rest opts)
> --
> 2.40.0
>

-- 
Philip Kaludercic
Details
Message ID
<87y1nx3dr5.fsf@treypeacock.com>
In-Reply-To
<87v8j1love.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
Patch: +30 -17
"Philip Kaludercic" <philipk@posteo.net> writes:

> I get the impression that cl-defmacro is too great of an overhead here
> -- especially when we consider loading the necessary files during
> initialisation.  `Setup' is relatively fast, and if there is a more
> efficient/lightweight way then I think it would be preferable.
> Dispatching ensure-specs could also be done using an alist mapping
> symbols to functions.

That makes sense. There should be a new patch attached below. I'm not
extremely familiar with the email patch workflow, so I hope that is okay

> That being said, I wonder if this is really so
> much potential for extending `setup--ensure' or if simply extending it
> from time to time is the good enough of a solution?

If `setup--ensure' were to be extended, would you need to require the
`keymap.el' library in this case?

My thoughts if you allow for user customization:
- Remove any future responsibility of adding to `setup--ensure'
- Additonally you could continue to add examples to the Wiki
for popular/commonly used specs.

It would also be quite useful for a few of the `package-vc' macros that
I'm currently working on.

> Especially when we keep in mind that for infrequent objects it is often
> just easier to preform the check yourself in the local macro instead of
> using an ensure-spec.

To your point, I guess it does not matter whether the logic sits in
`setup.el' or the macro itself. I just saw the ability added recently
and thought it would be useful to extend it.

Also, re: the patch. I wasn't sure if `setup-ensure-alist' should be a
`defcustom' or `defvar'.

---
diff --git a/setup.el b/setup.el
index 4574487..5698e5a 100644
--- a/setup.el
+++ b/setup.el
@@ -144,11 +144,32 @@ (defmacro setup (name &rest body)
;;;###autoload
(put 'setup 'function-documentation '(setup--make-docstring))

(defcustom setup-ensure-alist '((kbd . setup--ensure-kbd)
                                (func . setup--ensure-func)
                                (nil . identity))
  "Associative list assigning spec symbol to function.")

(defun setup--ensure-kbd (arg)
  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
  (cond
   ((stringp arg) (kbd arg))
   ((symbolp arg) `(kbd ,arg))
   (arg)))

(defun setup--ensure-func (arg)
  "Ensure ARG is a function, sharp-quoted if necessary."
  (cond
   ((eq (car-safe arg) 'function)
    arg)
   ((eq (car-safe arg) 'quote)
    `#',(cadr arg))
   ((symbolp arg)
    `#',arg)
   (arg)))

(defun setup--ensure (ensure-spec args &optional check-len)
  "Ensure that ARGS matches the form of ENSURE-SPEC.

The symbol `kbd' means to apply the function `kbd' to the
argument.  The symbol `func' means to sharp-quote the argument.
The symbol `&rest' means that the remaining elements of
ENSURE-SPEC are applied repeatedly to the remaining elements of
ARGS.
@@ -188,21 +209,13 @@ (defun setup--ensure (ensure-spec args &optional check-len)
                                        nil))
                  (push ensured result))))
          (let ((arg (pop args)))
            (push (cond ((null ensure) arg) ;Do not modify argument
                        ((eq ensure 'kbd) (cond
                                           ((stringp arg) (kbd arg))
                                           ((symbolp arg) `(kbd ,arg))
                                           (arg)))
                        ((eq ensure 'func) (cond
                                            ((eq (car-safe arg) 'function)
                                             arg)
                                            ((eq (car-safe arg) 'quote)
                                             `#',(cadr arg))
                                            ((symbolp arg)
                                             `#',arg)
                                            (arg)))
                        ((error "Invalid ensure spec %S" ensure)))
                  result)))))
            (push
             (let ((spec
                    (alist-get ensure setup-ensure-alist)))
               (if (functionp spec)
                   (funcall spec arg)
                 (error "Invalid ensure spec %S" ensure)))
             result)))))
    (nreverse result)))

(defun setup-define (name fn &rest opts)
Details
Message ID
<871qlp4tr0.fsf@treypeacock.com>
In-Reply-To
<87v8j1love.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
"Philip Kaludercic" <philipk@posteo.net> writes:

> Hi,
>
> Trey Peacock <git@treypeacock.com> writes:
>
>> I currently use a modified versions of the keybinding macros that use
>> `keymap-set` and other functions found in `keymap.el`. This resulted in
>> an error thrown when trying to use the new `kbd` ensure keyword. "C-="
>> can be set with `keymap-set`, but it will result an error in
>> `setup--ensure` with `(kbd "C-=")`.
>>
>> So, I figured it may be useful to allow for custom ENSURE-SPECs. The
>> implementation below uses `cl-defgeneric` to allow for custom methods;
>> however, if you do not want to rely on the `cl-lib` library, I have a
>> patch that uses an alist variable instead. This seemed a bit cleaner to
>> me.
>>
>> For example, this is the spec that I use for by custom `:bind` macros:
>>
>> (cl-defmethod setup-ensure-spec ((spec (eql key-valid)) arg)
>>   (if (key-valid-p arg)
>>       arg
>>     (setup-ensure-spec 'kbd arg)))
>>
>> Do you think this is something you would be interested in?
>
> I get the impression that cl-defmacro is too great of an overhead here
> -- especially when we consider loading the necessary files during
> initialisation.  `Setup' is relatively fast, and if there is a more
> efficient/lightweight way then I think it would be preferable.
> Dispatching ensure-specs could also be done using an alist mapping
> symbols to functions.  That being said, I wonder if this is really so
> much potential for extending `setup--ensure' or if simply extending it
> from time to time is the good enough of a solution?  Especially when we
> keep in mind that for infrequent objects it is often just easier to
> preform the check yourself in the local macro instead of using an
> ensure-spec.

That makes sense. There should be a new patch below. I'm not sure if
this is how I should submit it, I haven't done a lot of contributions
through email yet.

>
>>
>> ---
>>  setup.el | 48 +++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 31 insertions(+), 17 deletions(-)
>>
>> diff --git a/setup.el b/setup.el
>> index 4574487..1d4f1de 100644
>> --- a/setup.el
>> +++ b/setup.el
>> @@ -43,6 +43,7 @@
>>
>>  ;;; Code:
>>
>> +(require 'cl)
>
> Watch out, you want to load cl-generic, not cl -- that is deprecated.
>
>>  (require 'elisp-mode)
>>
>>  (defvar setup-opts `((quit . ,(make-symbol "setup-quit")))
>> @@ -144,11 +145,35 @@ (defmacro setup (name &rest body)
>>  ;;;###autoload
>>  (put 'setup 'function-documentation '(setup--make-docstring))
>>
>> +(cl-defgeneric setup-ensure-spec (ensure-spec arg)
>> +  "Ensure that ARGS matches the form of ENSURE-SPEC.
>> +Return nil by default."
>> +  nil)
>> +
>> +(cl-defmethod setup-ensure-spec ((_ (eql nil)) arg)
>> +  arg)
>> +
>> +(cl-defmethod setup-ensure-spec ((_ (eql kbd)) arg)
>> +  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
>> +  (cond
>> +   ((stringp arg) (kbd arg))
>> +   ((symbolp arg) `(kbd ,arg))
>> +   (arg)))
>> +
>> +(cl-defmethod setup-ensure-spec ((_ (eql func)) arg)
>> +  "Ensure ARG is a function, sharp-quoted if necessary."
>> +  (cond
>> +   ((eq (car-safe arg) 'function)
>> +    arg)
>> +   ((eq (car-safe arg) 'quote)
>> +    `#',(cadr arg))
>> +   ((symbolp arg)
>> +    `#',arg)
>> +   (arg)))
>> +
>>  (defun setup--ensure (ensure-spec args &optional check-len)
>>    "Ensure that ARGS matches the form of ENSURE-SPEC.
>>
>> -The symbol `kbd' means to apply the function `kbd' to the
>> -argument.  The symbol `func' means to sharp-quote the argument.
>>  The symbol `&rest' means that the remaining elements of
>>  ENSURE-SPEC are applied repeatedly to the remaining elements of
>>  ARGS.
>> @@ -188,21 +213,10 @@ (defun setup--ensure (ensure-spec args &optional check-len)
>>                                          nil))
>>                    (push ensured result))))
>>            (let ((arg (pop args)))
>> -            (push (cond ((null ensure) arg) ;Do not modify argument
>> -                        ((eq ensure 'kbd) (cond
>> -                                           ((stringp arg) (kbd arg))
>> -                                           ((symbolp arg) `(kbd ,arg))
>> -                                           (arg)))
>> -                        ((eq ensure 'func) (cond
>> -                                            ((eq (car-safe arg) 'function)
>> -                                             arg)
>> -                                            ((eq (car-safe arg) 'quote)
>> -                                             `#',(cadr arg))
>> -                                            ((symbolp arg)
>> -                                             `#',arg)
>> -                                            (arg)))
>> -                        ((error "Invalid ensure spec %S" ensure)))
>> -                  result)))))
>> +            (push
>> +             (or (setup-ensure-spec ensure arg)
>> +                 (error "Invalid ensure spec %S" ensure))
>> +             result)))))
>>      (nreverse result)))
>>
>>  (defun setup-define (name fn &rest opts)
>> --
>> 2.40.0
>>
>
> --
> Philip Kaludercic
--
Trey Peacock
treypeacock.com
Details
Message ID
<87ttyliu9f.fsf@posteo.net>
In-Reply-To
<871qlp4tr0.fsf@treypeacock.com> (view parent)
DKIM signature
missing
Download raw message
Trey Peacock <git@treypeacock.com> writes:

> "Philip Kaludercic" <philipk@posteo.net> writes:
>
>> Hi,
>>
>> Trey Peacock <git@treypeacock.com> writes:
>>
>>> I currently use a modified versions of the keybinding macros that use
>>> `keymap-set` and other functions found in `keymap.el`. This resulted in
>>> an error thrown when trying to use the new `kbd` ensure keyword. "C-="
>>> can be set with `keymap-set`, but it will result an error in
>>> `setup--ensure` with `(kbd "C-=")`.
>>>
>>> So, I figured it may be useful to allow for custom ENSURE-SPECs. The
>>> implementation below uses `cl-defgeneric` to allow for custom methods;
>>> however, if you do not want to rely on the `cl-lib` library, I have a
>>> patch that uses an alist variable instead. This seemed a bit cleaner to
>>> me.
>>>
>>> For example, this is the spec that I use for by custom `:bind` macros:
>>>
>>> (cl-defmethod setup-ensure-spec ((spec (eql key-valid)) arg)
>>>   (if (key-valid-p arg)
>>>       arg
>>>     (setup-ensure-spec 'kbd arg)))
>>>
>>> Do you think this is something you would be interested in?
>>
>> I get the impression that cl-defmacro is too great of an overhead here
>> -- especially when we consider loading the necessary files during
>> initialisation.  `Setup' is relatively fast, and if there is a more
>> efficient/lightweight way then I think it would be preferable.
>> Dispatching ensure-specs could also be done using an alist mapping
>> symbols to functions.  That being said, I wonder if this is really so
>> much potential for extending `setup--ensure' or if simply extending it
>> from time to time is the good enough of a solution?  Especially when we
>> keep in mind that for infrequent objects it is often just easier to
>> preform the check yourself in the local macro instead of using an
>> ensure-spec.
>
> That makes sense. There should be a new patch below. I'm not sure if
> this is how I should submit it, I haven't done a lot of contributions
> through email yet.

Eh, it seems you forgot to add the patch? ^^

In general, mailing-list driven development is usually flexible enough.
As you see, if anything goes wrong or I can't work with what you sent
me, I just ask you resend the message.
Details
Message ID
<87sfe5f1mh.fsf@posteo.net>
In-Reply-To
<87y1nx3dr5.fsf@treypeacock.com> (view parent)
DKIM signature
missing
Download raw message
Trey Peacock <git@treypeacock.com> writes:

> "Philip Kaludercic" <philipk@posteo.net> writes:
>
>> I get the impression that cl-defmacro is too great of an overhead here
>> -- especially when we consider loading the necessary files during
>> initialisation.  `Setup' is relatively fast, and if there is a more
>> efficient/lightweight way then I think it would be preferable.
>> Dispatching ensure-specs could also be done using an alist mapping
>> symbols to functions.
>
> That makes sense. There should be a new patch attached below. I'm not
> extremely familiar with the email patch workflow, so I hope that is okay

As I mentioned in my other message, don't worry about it.  Just remember
to CC the mailing list by using some kind of a wide-reply so that the
conversation gets recorded on the mailing list.

>> That being said, I wonder if this is really so
>> much potential for extending `setup--ensure' or if simply extending it
>> from time to time is the good enough of a solution?
>
> If `setup--ensure' were to be extended, would you need to require the
> `keymap.el' library in this case?

We could also just require it if these ensure specs are used, and as I
don't intend to raise the minimum version of setup.el that soon, this
couldn't be a hard dependency.

> My thoughts if you allow for user customization:
> - Remove any future responsibility of adding to `setup--ensure'
> - Additonally you could continue to add examples to the Wiki
> for popular/commonly used specs.

Yeah, but that is my point.  What other specs do you think there are
that should be added.  My intuition is that there is an upper bound to
how many there can be, but I might be wrong.

> It would also be quite useful for a few of the `package-vc' macros that
> I'm currently working on.

Oh, for example?

>> Especially when we keep in mind that for infrequent objects it is often
>> just easier to preform the check yourself in the local macro instead of
>> using an ensure-spec.
>
> To your point, I guess it does not matter whether the logic sits in
> `setup.el' or the macro itself. I just saw the ability added recently
> and thought it would be useful to extend it.
>
> Also, re: the patch. I wasn't sure if `setup-ensure-alist' should be a
> `defcustom' or `defvar'.

Usually I would say a defcustom, but setup hasn't been using any user
options until now so a defvar is ok, even if it is more low-level.

> ---
> diff --git a/setup.el b/setup.el
> index 4574487..5698e5a 100644
> --- a/setup.el
> +++ b/setup.el
> @@ -144,11 +144,32 @@ (defmacro setup (name &rest body)
>  ;;;###autoload
>  (put 'setup 'function-documentation '(setup--make-docstring))
>
> +(defcustom setup-ensure-alist '((kbd . setup--ensure-kbd)
> +                                (func . setup--ensure-func)

I would use a backquote-here, then unquote the functions and sharp-quote
them again so that the byte-compiler can detect if they are renamed or
otherwise missing.

> +                                (nil . identity))

Not sure about the usage of nil here... We can also just rely on
alist-get to provide a default function (identity) if nothing else is
missing?

> +  "Associative list assigning spec symbol to function.")
      ^
      "Alist" should be brief enough.
                        ^
                        mapping?

It would also be nice to go into some more detail what this a "spec
symbol" is even supposed to be, for users who have never heard of these
things before.  Also, how should the function behave (how many arguments
of what kind, what does it return, side-effects or anything like that).

> +
> +(defun setup--ensure-kbd (arg)
> +  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
> +  (cond
> +   ((stringp arg) (kbd arg))
> +   ((symbolp arg) `(kbd ,arg))
> +   (arg)))
> +
> +(defun setup--ensure-func (arg)
> +  "Ensure ARG is a function, sharp-quoted if necessary."
> +  (cond
> +   ((eq (car-safe arg) 'function)
> +    arg)

The lines here are short-enough to be folded.

> +   ((eq (car-safe arg) 'quote)
> +    `#',(cadr arg))
> +   ((symbolp arg)
> +    `#',arg)
> +   (arg)))
> +
>  (defun setup--ensure (ensure-spec args &optional check-len)
>    "Ensure that ARGS matches the form of ENSURE-SPEC.
>
> -The symbol `kbd' means to apply the function `kbd' to the
> -argument.  The symbol `func' means to sharp-quote the argument.

No reference to `setup-ensure-alist'?

>  The symbol `&rest' means that the remaining elements of
>  ENSURE-SPEC are applied repeatedly to the remaining elements of
>  ARGS.
> @@ -188,21 +209,13 @@ (defun setup--ensure (ensure-spec args &optional check-len)
>                                          nil))
>                    (push ensured result))))
>            (let ((arg (pop args)))
> -            (push (cond ((null ensure) arg) ;Do not modify argument
> -                        ((eq ensure 'kbd) (cond
> -                                           ((stringp arg) (kbd arg))
> -                                           ((symbolp arg) `(kbd ,arg))
> -                                           (arg)))
> -                        ((eq ensure 'func) (cond
> -                                            ((eq (car-safe arg) 'function)
> -                                             arg)
> -                                            ((eq (car-safe arg) 'quote)
> -                                             `#',(cadr arg))
> -                                            ((symbolp arg)
> -                                             `#',arg)
> -                                            (arg)))
> -                        ((error "Invalid ensure spec %S" ensure)))
> -                  result)))))
> +            (push
> +             (let ((spec
> +                    (alist-get ensure setup-ensure-alist)))
> +               (if (functionp spec)
> +                   (funcall spec arg)
> +                 (error "Invalid ensure spec %S" ensure)))
> +             result)))))
>      (nreverse result)))
>
>  (defun setup-define (name fn &rest opts)

Otherwise this looks good, when we address these issues I can push the
patch.  Do you want to also add a change entry under the "News" section
in the package header?

BTW, have you signed the FSF CA?  Since this is an ELPA package, I am
required to make sure that all significant contributors have done so.
Details
Message ID
<878rfrox5k.fsf@posteo.net>
In-Reply-To
<87sfe5f1mh.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
ping?

Philip Kaludercic <philipk@posteo.net> writes:

> Trey Peacock <git@treypeacock.com> writes:
>
>> "Philip Kaludercic" <philipk@posteo.net> writes:
>>
>>> I get the impression that cl-defmacro is too great of an overhead here
>>> -- especially when we consider loading the necessary files during
>>> initialisation.  `Setup' is relatively fast, and if there is a more
>>> efficient/lightweight way then I think it would be preferable.
>>> Dispatching ensure-specs could also be done using an alist mapping
>>> symbols to functions.
>>
>> That makes sense. There should be a new patch attached below. I'm not
>> extremely familiar with the email patch workflow, so I hope that is okay
>
> As I mentioned in my other message, don't worry about it.  Just remember
> to CC the mailing list by using some kind of a wide-reply so that the
> conversation gets recorded on the mailing list.
>
>>> That being said, I wonder if this is really so
>>> much potential for extending `setup--ensure' or if simply extending it
>>> from time to time is the good enough of a solution?
>>
>> If `setup--ensure' were to be extended, would you need to require the
>> `keymap.el' library in this case?
>
> We could also just require it if these ensure specs are used, and as I
> don't intend to raise the minimum version of setup.el that soon, this
> couldn't be a hard dependency.
>
>> My thoughts if you allow for user customization:
>> - Remove any future responsibility of adding to `setup--ensure'
>> - Additonally you could continue to add examples to the Wiki
>> for popular/commonly used specs.
>
> Yeah, but that is my point.  What other specs do you think there are
> that should be added.  My intuition is that there is an upper bound to
> how many there can be, but I might be wrong.
>
>> It would also be quite useful for a few of the `package-vc' macros that
>> I'm currently working on.
>
> Oh, for example?
>
>>> Especially when we keep in mind that for infrequent objects it is often
>>> just easier to preform the check yourself in the local macro instead of
>>> using an ensure-spec.
>>
>> To your point, I guess it does not matter whether the logic sits in
>> `setup.el' or the macro itself. I just saw the ability added recently
>> and thought it would be useful to extend it.
>>
>> Also, re: the patch. I wasn't sure if `setup-ensure-alist' should be a
>> `defcustom' or `defvar'.
>
> Usually I would say a defcustom, but setup hasn't been using any user
> options until now so a defvar is ok, even if it is more low-level.
>
>> ---
>> diff --git a/setup.el b/setup.el
>> index 4574487..5698e5a 100644
>> --- a/setup.el
>> +++ b/setup.el
>> @@ -144,11 +144,32 @@ (defmacro setup (name &rest body)
>>  ;;;###autoload
>>  (put 'setup 'function-documentation '(setup--make-docstring))
>>
>> +(defcustom setup-ensure-alist '((kbd . setup--ensure-kbd)
>> +                                (func . setup--ensure-func)
>
> I would use a backquote-here, then unquote the functions and sharp-quote
> them again so that the byte-compiler can detect if they are renamed or
> otherwise missing.
>
>> +                                (nil . identity))
>
> Not sure about the usage of nil here... We can also just rely on
> alist-get to provide a default function (identity) if nothing else is
> missing?
>
>> +  "Associative list assigning spec symbol to function.")
>       ^
>       "Alist" should be brief enough.
>                         ^
>                         mapping?
>
> It would also be nice to go into some more detail what this a "spec
> symbol" is even supposed to be, for users who have never heard of these
> things before.  Also, how should the function behave (how many arguments
> of what kind, what does it return, side-effects or anything like that).
>
>> +
>> +(defun setup--ensure-kbd (arg)
>> +  "Ensure ARG is valid `kbd' syntax, applying `kbd' if necessary."
>> +  (cond
>> +   ((stringp arg) (kbd arg))
>> +   ((symbolp arg) `(kbd ,arg))
>> +   (arg)))
>> +
>> +(defun setup--ensure-func (arg)
>> +  "Ensure ARG is a function, sharp-quoted if necessary."
>> +  (cond
>> +   ((eq (car-safe arg) 'function)
>> +    arg)
>
> The lines here are short-enough to be folded.
>
>> +   ((eq (car-safe arg) 'quote)
>> +    `#',(cadr arg))
>> +   ((symbolp arg)
>> +    `#',arg)
>> +   (arg)))
>> +
>>  (defun setup--ensure (ensure-spec args &optional check-len)
>>    "Ensure that ARGS matches the form of ENSURE-SPEC.
>>
>> -The symbol `kbd' means to apply the function `kbd' to the
>> -argument.  The symbol `func' means to sharp-quote the argument.
>
> No reference to `setup-ensure-alist'?
>
>>  The symbol `&rest' means that the remaining elements of
>>  ENSURE-SPEC are applied repeatedly to the remaining elements of
>>  ARGS.
>> @@ -188,21 +209,13 @@ (defun setup--ensure (ensure-spec args &optional check-len)
>>                                          nil))
>>                    (push ensured result))))
>>            (let ((arg (pop args)))
>> -            (push (cond ((null ensure) arg) ;Do not modify argument
>> -                        ((eq ensure 'kbd) (cond
>> -                                           ((stringp arg) (kbd arg))
>> -                                           ((symbolp arg) `(kbd ,arg))
>> -                                           (arg)))
>> -                        ((eq ensure 'func) (cond
>> -                                            ((eq (car-safe arg) 'function)
>> -                                             arg)
>> -                                            ((eq (car-safe arg) 'quote)
>> -                                             `#',(cadr arg))
>> -                                            ((symbolp arg)
>> -                                             `#',arg)
>> -                                            (arg)))
>> -                        ((error "Invalid ensure spec %S" ensure)))
>> -                  result)))))
>> +            (push
>> +             (let ((spec
>> +                    (alist-get ensure setup-ensure-alist)))
>> +               (if (functionp spec)
>> +                   (funcall spec arg)
>> +                 (error "Invalid ensure spec %S" ensure)))
>> +             result)))))
>>      (nreverse result)))
>>
>>  (defun setup-define (name fn &rest opts)
>
> Otherwise this looks good, when we address these issues I can push the
> patch.  Do you want to also add a change entry under the "News" section
> in the package header?
>
> BTW, have you signed the FSF CA?  Since this is an ELPA package, I am
> required to make sure that all significant contributors have done so.

-- 
Philip Kaludercic
Reply to thread Export thread (mbox)