alphapapa / emacs-package-dev-handbook

An Emacs package development handbook. Built with Emacs, by Emacs package developers, for Emacs package developers.
GNU General Public License v3.0
1.13k stars 45 forks source link

Replace `with-plist-vars` #7

Open Luis-Henriquez-Perez opened 5 years ago

Luis-Henriquez-Perez commented 5 years ago

First, thanks for this handbook. I haven't developed any emacs packages (yet), but even for my own init.el customizations this has been invaluable.

I was using the with-plist-vars macro and I liked it. However, I found that it had some significant shortcomings.

One is that it requires a raw plist and will not work with a variable which contains a plist. For instance (progn (setq plist '(:hi 1 :ho 2)) (with-plist-vars plist (+ hi ho)) does not work.

Equally problematic was the fact that it used variables with the same name as the keys. These variables are likely to clobber other variables because they're not prefixed by anything. Additionally, often times I want to try to get a value from a key that I don't know is in the plist. Since there is no special notation between a variable that refers to a key and any other variable I have no way of doing this.

I would recommend adding this macro instead to your handbook:

(defmacro let-plist (plist &rest body)
  "Let-bind dotted symbols to their value in PLIST and execute BODY.
Dotted symbol is any symbol starting with a ..  Only those present
in BODY are let-bound and this search is done at compile time.

For instance, the following code

  (let-plist plist
    (if (and .title .body)
        .body
      .site))

essentially expands to

  (let ((.title (plist-get plist :title))
        (.body  (plist-get plist :body))
        (.site  (plist-get plist :site)))
    (if (and .title .body)
        .body
      .site))
"
  (declare (indent defun))
  (require 'let-alist)
  (let ((plist-var (gensym)))
    `(let* 
         ((,plist-var ,(if (and (listp plist) (not (eq (car plist) 'quote)))
                           `(quote ,plist)
                         plist))
          ,@(cl-loop for (dotvar . var) in (let-alist--deep-dot-search body)
                     for key = (intern (concat ":" (symbol-name var)))
                     collect `(,dotvar (plist-get ,plist-var ,key))))
       ,@body)))

This macro addresses both these misgivings and is consistent with the let-alist macro already built-in to 26.1. It uses let-alist--deep-dot-search-body to get the variables which are prefixed by a ..

EDIT: fixed leak with plist var. Before the edit, if you passed in some expression which would evaluate to a plist, the expression would be evaluated one time for every unique dot variable. If the expression had side-effects or if it was computationally expensive, this could lead to unexpected results. Also I check to see if plist is an unquoted list and if it is I quote it. This is for convenience so that (let-plist (:hi 1 :ho 2) ...) will work even though the raw plist isn't quoted.

EDIT: Add (require 'let-alist).

alphapapa commented 5 years ago

Hi,

Thanks for the kind words. Glad you've found it useful.

This is a really great idea. In fact, it's so good that I think it would be a good addition to Emacs itself. Have you considered proposing that? Maybe you could post it to /r/emacs first to get feedback, and then propose it on emacs-devel.

I'll be glad to add it to the handbook in the meantime.

Luis-Henriquez-Perez commented 5 years ago

I hadn't considered proposing it. Posting it sounds like a great idea.

I think then it'd be best to wait until I post it onto /r/emacs to get feedback before adding it to the handbook. Testing it, I've found some improvements I'd like to make. One, for example, is both let-plist and let-alist don't work when you try to access the alist/plist values from inside a backquote.

Example:

;; This doesn't work.
(let-alist '((hi . 1) (ho . 2))
  `(+ ,.hi ,.ho))
;; I expect this to be `(+ 1 2)'. 
;; Instead it's `(+ (,. hi) (,. ho))'

As for proposing it on emacs-devel, I'm not sure. Perhaps would it be better off as part of a package? I'm conservative about adding stuff to emacs itself because I think something that might seem very useful to me (or even most emacs users) might not be wanted for some. But with a package those who want it can install it; and those who don't can leave it. I don't want to force anything on anyone.

Then again though, let-alist is already there and it is strange having it there without a plist counterpart. So I'm uncertain.

alphapapa commented 5 years ago

I think then it'd be best to wait until I post it onto /r/emacs to get feedback before adding it to the handbook. Testing it, I've found some improvements I'd like to make.

Sounds good. Please post an update here when you think it's ready, in case I miss the Reddit thread.

One, for example, is both let-plist and let-alist don't work when you try to access the alist/plist values from inside a backquote.

Example:

;; This doesn't work.
(let-alist '((hi . 1) (ho . 2))
  `(+ ,.hi ,.ho))
;; I expect this to be `(+ 1 2)'. 
;; Instead it's `(+ (,. hi) (,. ho))'

That's interesting. In my limited experiments with rewriting backquoted and unquoted lists, I found that it's very confusing, and I was unsuccessful. It seems to be a bit of a "black art". I suppose if one studied the relevant Emacs source code that already does this, one might eventually come to understand it--maybe someday... :)

As for proposing it on emacs-devel, I'm not sure. Perhaps would it be better off as part of a package? I'm conservative about adding stuff to emacs itself because I think something that might seem very useful to me (or even most emacs users) might not be wanted for some. But with a package those who want it can install it; and those who don't can leave it. I don't want to force anything on anyone.

That's a good point. AFAIK let-alist is also an ELPA package, as well as being in Emacs core (which lets older Emacs versions use it). It might suit let-plist to be an ELPA package first, and eventually go into core if desired.

Then again though, let-alist is already there and it is strange having it there without a plist counterpart. So I'm uncertain.

It's also strange that map.el doesn't support plists. I can't remember for certain, but I think a patch was merged in the last few months addressing that...

Luis-Henriquez-Perez commented 5 years ago

I just posted the latest code for it on reddit. I hope it will be received well as it was my first reddit post. I made an account just to post this :)

Luis-Henriquez-Perez commented 5 years ago

For my latest version of let-plist I used a modified version of the stackoverflow answer mention on reddit.

The stackoverflow answer only worked on plists whose keys are keywords (I know you mentioned you didn't like this and neither do I) so I have it work for both symbols and keywords.

One big difference I made is I abstracted the prefix symbol into a variable. This way, if you prefer the . notation and don't care about the backquote issue you can use it. But if you do (like me) then you can use a different symbol (like $). Additionally, you can change how let-plist works on the fly just by changing the value of let-plist--symbol-prefix.

I am considering adding an option for shorthand syntax when accessing plist values that are keys (something shorter than .:key (assuming . notation). But I'm not sure exactly what that syntax should be.

As it stands .key is used to access a key in plist that's a symbol and .:key is used to access a key in plist that's a keyword.

(defvar let-plist--symbol-prefix "$")

(defun let-plist--remove-prefix (symbol)
  "Return SYMBOL, without an initial prefix.
Which prefix in particular is `let-plist--symbol-prefix'."
  (let ((name (symbol-name symbol))
        (regexp (eval `(rx bos ,let-plist--symbol-prefix) t)))
    (if (string-match regexp name)
        (intern (replace-match "" nil nil name))
      symbol)))

(defun let-plist--list-to-sexp (list var)
  "Turn symbols LIST into recursive calls to `plist-get' on VAR."
  `(plist-get ,(if (cdr list)
                   (let-plist--list-to-sexp (cdr list) var)
                 var)
              ',(car list)))

(defun let-plist--access-sexp (symbol variable)
  "Return a sexp used to access SYMBOL inside VARIABLE."
  (let* ((clean (let-plist--remove-prefix symbol))
         (name (symbol-name clean))
         (regexp (eval `(rx bol ,let-plist--symbol-prefix) t)))
    (if (string-match regexp name)
        clean
      (let-plist--list-to-sexp
       (mapcar #'intern (thread-last (eval `(rx ,let-plist--symbol-prefix) t)
                          (split-string name)
                          (nreverse)))
       variable))))

(defun let-plist--deep-var-search (data)
  "Return alist of symbols inside DATA that start with
`let-plist--symbol-prefix'. Perform a deep search and return an alist where each
car is the symbol, and each cdr is the same symbol without the
`let-plist--symbol-prefix'."
  (cond
   ((symbolp data)
    (let ((name (symbol-name data))
          (regexp (eval `(rx bol ,let-plist--symbol-prefix) t)))
      (when (string-match regexp name)
        ;; Return the cons cell inside a list, so it can be appended
        ;; with other results in the clause below.
        (list (cons data (intern (replace-match "" nil nil name)))))))
   ((not (consp data)) nil)
   ((eq (car data) 'let-alist)
    ;; For nested ‘let-alist’ forms, ignore symbols appearing in the
    ;; inner body because they don’t refer to the alist currently
    ;; being processed.  See Bug#24641.
    (let-plist--deep-var-search (cadr data)))
   (t (append (let-plist--deep-var-search (car data))
              (let-plist--deep-var-search (cdr data))))))

(defmacro let-plist (plist &rest body)
  (declare (indent 1))
  (let ((plist-var (gensym)))
    `(let ((,plist-var ,plist))
       (let ,(cl-loop for (dotvar . var) in (delete-dups (let-plist--deep-var-search body))
                      collect `(,dotvar ,(let-plist--access-sexp dotvar plist-var)))
         ,@body))))