map.el: pcase plist keyword-only binding improvement

map.el: pcase plist keyword-only binding improvement

[Adam Porter]

Hi,

One of the features I find very useful in dash.el's `-let' macro is its
ability to bind plist values to variables of the same names as the keys.
With map.el's `pcase' support, Emacs's `pcase' and `pcase-let' can do
this as well, but in a less convenient way, because it requires writing
both the keyword and the variable name.

However, with a simple change to one of map'el's functions, it can
support keyword-only plist binding as well.

Example usage before change:

#+BEGIN_SRC elisp
  (let ((pl '(:one 1 :two 2)))
    (pcase pl
      ((map :one (:two two))
       (list one two))))  ;;=> nil

  (pcase-let* (((map :one (:two two))
                '(:one 1 :two 2)))
    (list one two)) ;;=> (void-variable one)
#+END_SRC

Changed function:

#+BEGIN_SRC elisp
  (defun map--make-pcase-bindings (args)
    "Return a list of pcase bindings from ARGS to the elements of a map."
    (seq-map (lambda (elt)
               (cond ((consp elt)
                      `(app (pcase--flip map-elt ,(car elt)) ,(cadr elt)))
                     ((keywordp elt)
                      ;; New clause for keyword-only elements.
                      (let ((var (intern (substring (symbol-name elt) 1))))
                        `(app (pcase--flip map-elt ,elt) ,var)))
                     (t
                      `(app (pcase--flip map-elt ',elt) ,elt))))
             args))
#+END_SRC

Example usage after change:

#+BEGIN_SRC elisp
  (let ((pl '(:one 1 :two 2)))
    (pcase pl
      ((map :one (:two two))
       (list one two)))) ;;=> (1 2)

  (pcase-let* (((map :one (:two two))
                '(:one 1 :two 2)))
    (list one two)) ;;=> (1 2)
#+END_SRC

Assuming that this change is acceptable, I would like to prepare a
patch, which would probably need to include NEWS and documentation
changes.  However, since the change is to map.el rather than pcase.el,
and since map.el is also available on ELPA, I'm not sure how to proceed
with those parts, where to make the appropriate documentation changes,
etc.

Please let me know if this proposal is acceptable and how I should
proceed.

Thanks,
Adam

Re: map.el: pcase plist keyword-only binding improvement

[Stefan Monnier]

>     (seq-map (lambda (elt)
>                (cond ((consp elt)
>                       `(app (pcase--flip map-elt ,(car elt)) ,(cadr elt)))
>                      ((keywordp elt)
>                       ;; New clause for keyword-only elements.
>                       (let ((var (intern (substring (symbol-name elt) 1))))
>                         `(app (pcase--flip map-elt ,elt) ,var)))
>                      (t
>                       `(app (pcase--flip map-elt ',elt) ,elt))))
>              args))

Since binding the result of the match to a keyword can't work anyway,
this seems like a very good idea.

> Assuming that this change is acceptable, I would like to prepare a
> patch, which would probably need to include NEWS and documentation
> changes.

AFAICT we don't yet have actual documentation for the map.el package, so
only NEWS needs to be changed.

> However, since the change is to map.el rather than pcase.el,
> and since map.el is also available on ELPA,

It's also distributed via GNU ELPA indeed, but taken straight from the
emacs.git repository, so you don't need to worry about that (just
increment the `Version:` entry at the top).


        Stefan

Re: map.el: pcase plist keyword-only binding improvement

[Adam Porter]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     (seq-map (lambda (elt)
>>                (cond ((consp elt)
>>                       `(app (pcase--flip map-elt ,(car elt)) ,(cadr elt)))
>>                      ((keywordp elt)
>>                       ;; New clause for keyword-only elements.
>>                       (let ((var (intern (substring (symbol-name elt) 1))))
>>                         `(app (pcase--flip map-elt ,elt) ,var)))
>>                      (t
>>                       `(app (pcase--flip map-elt ',elt) ,elt))))
>>              args))
>
> Since binding the result of the match to a keyword can't work anyway,
> this seems like a very good idea.
>
>> Assuming that this change is acceptable, I would like to prepare a
>> patch, which would probably need to include NEWS and documentation
>> changes.
>
> AFAICT we don't yet have actual documentation for the map.el package, so
> only NEWS needs to be changed.
>
>> However, since the change is to map.el rather than pcase.el,
>> and since map.el is also available on ELPA,
>
> It's also distributed via GNU ELPA indeed, but taken straight from the
> emacs.git repository, so you don't need to worry about that (just
> increment the `Version:` entry at the top).

Hi Stefan,

Ok, I've attached a patch.  I tried to follow the appropriate
guidelines, but please let me know if any changes are required.

Thanks,
Adam

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: map.el keyword-only pattern abbreviation --]
[-- Type: text/x-diff, Size: 3465 bytes --]

From d5cb310d6fb30e2991a94e433b754c07893133ef Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Sun, 2 Feb 2020 10:17:20 -0600
Subject: [PATCH] * lisp/emacs-lisp/map.el: Add keyword-only pattern
 abbreviation

* lisp/emacs-lisp/map.el: Update version to 2.1.
((pcase-defmacro map)): Update docstring.
(map--make-pcase-bindings): Match keyword pattern.

* test/lisp/emacs-lisp/map-tests.el (test-map-plist-pcase): Add test.
---
 etc/NEWS                          |  6 ++++++
 lisp/emacs-lisp/map.el            | 17 +++++++++++------
 test/lisp/emacs-lisp/map-tests.el |  6 ++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 2b9337b..7444166 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -107,6 +107,12 @@ supplied error message.
 *** New connection method "media", which allows accessing media devices
 like cell phones, tablets or cameras.
 
+** map.el
+
+*** Pcase 'map' pattern added keyword symbols abbreviation.
+A pattern like '(map :sym)' binds the map's value for ':sym' to 'sym',
+equivalent to '(map (:sym sym))'.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/emacs-lisp/map.el b/lisp/emacs-lisp/map.el
index 67f5b3c..9c23344 100644
--- a/lisp/emacs-lisp/map.el
+++ b/lisp/emacs-lisp/map.el
@@ -4,7 +4,7 @@
 
 ;; Author: Nicolas Petton <nicolas@petton.fr>
 ;; Keywords: convenience, map, hash-table, alist, array
-;; Version: 2.0
+;; Version: 2.1
 ;; Package-Requires: ((emacs "25"))
 ;; Package: map
 
@@ -56,8 +56,10 @@ evaluated and searched for in the map.  The match fails if for any KEY
 found in the map, the corresponding PAT doesn't match the value
 associated to the KEY.
 
-Each element can also be a SYMBOL, which is an abbreviation of a (KEY
-PAT) tuple of the form (\\='SYMBOL SYMBOL).
+Each element can also be a SYMBOL, which is an abbreviation of
+a (KEY PAT) tuple of the form (\\='SYMBOL SYMBOL).  When SYMBOL
+is a keyword, it is an abbreviation of the form (:SYMBOL SYMBOL),
+useful for binding plist values.
 
 Keys in ARGS not found in the map are ignored, and the match doesn't
 fail."
@@ -486,9 +488,12 @@ Example:
 (defun map--make-pcase-bindings (args)
   "Return a list of pcase bindings from ARGS to the elements of a map."
   (seq-map (lambda (elt)
-             (if (consp elt)
-                 `(app (pcase--flip map-elt ,(car elt)) ,(cadr elt))
-               `(app (pcase--flip map-elt ',elt) ,elt)))
+             (cond ((consp elt)
+                    `(app (pcase--flip map-elt ,(car elt)) ,(cadr elt)))
+                   ((keywordp elt)
+                    (let ((var (intern (substring (symbol-name elt) 1))))
+                      `(app (pcase--flip map-elt ,elt) ,var)))
+                   (t `(app (pcase--flip map-elt ',elt) ,elt))))
            args))
 
 (defun map--make-pcase-patterns (args)
diff --git a/test/lisp/emacs-lisp/map-tests.el b/test/lisp/emacs-lisp/map-tests.el
index 06fd55f..3ffef17 100644
--- a/test/lisp/emacs-lisp/map-tests.el
+++ b/test/lisp/emacs-lisp/map-tests.el
@@ -376,5 +376,11 @@ Evaluate BODY for each created map.
                                  '((1 . 1) (2 . 5) (3 . 0)))
                  '((3 . 0) (2 . 9) (1 . 6)))))
 
+(ert-deftest test-map-plist-pcase ()
+  (let ((plist '(:one 1 :two 2)))
+    (should (equal (pcase-let (((map :one (:two two)) plist))
+                     (list one two))
+                   '(1 2)))))
+
 (provide 'map-tests)
 ;;; map-tests.el ends here
-- 
2.7.4

Re: map.el: pcase plist keyword-only binding improvement

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hi Adam,

Hi Michael, Adam,

> I'm just CC'ing (offlist) Nicolas Petton, the author of map.el, who
> wanted to be kept informed about changes.  Please give him a few days
> and keep him CC'd in the discussion in emacs-dev.  He used to be short
> on time so it can happen he doesn't chime in.

Thanks for keeping me posted.

The change looks good, thanks!

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: map.el: pcase plist keyword-only binding improvement

[Stefan Monnier]

> Ok, I've attached a patch.  I tried to follow the appropriate
> guidelines, but please let me know if any changes are required.

Looks good, thanks, pushed,


        Stefan

Re: map.el: pcase plist keyword-only binding improvement

[Adam Porter]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Looks good, thanks, pushed,

Thanks!