bug#58720: 29.0.50; Improve error reporting of EUDC plist functions

bug#58720: 29.0.50; Improve error reporting of EUDC plist functions

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Severity: wishlist
Tags: patch

In https://bugs.gnu.org/58531#19 it was suggested that
eudc-plist-member, eudc-plist-get, and eudc-lax-plist-get could signal a
more informative wrong-type-argument instead of a generic static error
when passed a degenerate plist argument.  The attached patch makes this
so.

An alternative is to simply call plist-member and plist-get directly,
relying on them for any error reporting, and not worry about arguments
that degenerate further down the list than the point of interest.

WDYT?  How important is it to name and shame degenerate plists as
eagerly as possible in EUDC?

Thanks,

-- 
Basil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-error-reporting-of-EUDC-plist-functions.patch --]
[-- Type: text/x-diff, Size: 3919 bytes --]

From e9e447214b2b0d19af8b7dc43abbcd4c7944a065 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 22 Oct 2022 20:25:54 +0300
Subject: [PATCH] Improve error reporting of EUDC plist functions

* lisp/net/eudc.el (eudc--plist-member): Signal a more informative
wrong-type-argument instead of a generic error (bug#58531#19).
* test/lisp/net/eudc-tests.el (eudc--plist-member)
(eudc-plist-member, eudc-plist-get, eudc-lax-plist-get): Update
tests accordingly.
---
 lisp/net/eudc.el            |  5 ++---
 test/lisp/net/eudc-tests.el | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/lisp/net/eudc.el b/lisp/net/eudc.el
index 0283b04574..5f9e78fc7f 100644
--- a/lisp/net/eudc.el
+++ b/lisp/net/eudc.el
@@ -108,9 +108,8 @@ eudc--using-bbdb-3-or-newer-p
 
 (defun eudc--plist-member (plist prop &optional predicate)
   "Like `plist-member', but signal on invalid PLIST."
-  ;; Could also use `plistp', but that would change the error.
-  (or (zerop (% (length plist) 2))
-      (error "Malformed plist"))
+  (or (plistp plist)
+      (signal 'wrong-type-argument `(plistp ,plist)))
   (plist-member plist prop predicate))
 
 (defun eudc-plist-member (plist prop)
diff --git a/test/lisp/net/eudc-tests.el b/test/lisp/net/eudc-tests.el
index 219c250bf0..915006a97c 100644
--- a/test/lisp/net/eudc-tests.el
+++ b/test/lisp/net/eudc-tests.el
@@ -26,9 +26,9 @@ eudc--plist-member
   (dolist (obj '(a (a . a) (a a . a)))
     (should-error (eudc--plist-member obj nil) :type 'wrong-type-argument))
   (dolist (plist '((nil) (a) (a a a)))
-    (dolist (key '(nil a))
-      (should (equal (should-error (eudc--plist-member plist key))
-                     '(error "Malformed plist")))))
+    (let ((err `(wrong-type-argument plistp ,(copy-sequence plist))))
+      (dolist (key '(nil a))
+        (should (equal err (should-error (eudc--plist-member plist key)))))))
   (let ((-nil (string ?n ?i ?l))
         (-a (string ?a)))
     (should-not (eudc--plist-member () nil))
@@ -56,9 +56,9 @@ eudc-plist-member
   (dolist (obj '(a (a . a) (a a . a)))
     (should-error (eudc-plist-member obj nil) :type 'wrong-type-argument))
   (dolist (plist '((nil) (a) (a a a)))
-    (dolist (key '(nil a))
-      (should (equal (should-error (eudc-plist-member plist key))
-                     '(error "Malformed plist")))))
+    (let ((err `(wrong-type-argument plistp ,(copy-sequence plist))))
+      (dolist (key '(nil a))
+        (should (equal err (should-error (eudc-plist-member plist key)))))))
   (let ((-nil (string ?n ?i ?l))
         (-a (string ?a)))
     (should-not (eudc-plist-member () nil))
@@ -86,9 +86,9 @@ eudc-plist-get
   (dolist (obj '(a (a . a) (a a . a)))
     (should-error (eudc-plist-get obj nil) :type 'wrong-type-argument))
   (dolist (plist '((nil) (a) (a a a)))
-    (dolist (key '(nil a))
-      (should (equal (should-error (eudc-plist-get plist key))
-                     '(error "Malformed plist")))))
+    (let ((err `(wrong-type-argument plistp ,(copy-sequence plist))))
+      (dolist (key '(nil a))
+        (should (equal err (should-error (eudc-plist-get plist key)))))))
   (let ((-nil (string ?n ?i ?l))
         (-a (string ?a)))
     (should-not (eudc-plist-get () nil))
@@ -120,9 +120,9 @@ eudc-lax-plist-get
   (dolist (obj '(a (a . a) (a a . a)))
     (should-error (eudc-lax-plist-get obj nil) :type 'wrong-type-argument))
   (dolist (plist '((nil) (a) (a a a)))
-    (dolist (key '(nil a))
-      (should (equal (should-error (eudc-lax-plist-get plist key))
-                     '(error "Malformed plist")))))
+    (let ((err `(wrong-type-argument plistp ,(copy-sequence plist))))
+      (dolist (key '(nil a))
+        (should (equal err (should-error (eudc-lax-plist-get plist key)))))))
   (let ((-nil (string ?n ?i ?l))
         (-a (string ?a)))
     (should-not (eudc-lax-plist-get () nil))
-- 
2.35.1

bug#58720: 29.0.50; Improve error reporting of EUDC plist functions

[Thomas Fitzsimmons]

Hi Basil,

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Severity: wishlist
> Tags: patch
>
> In https://bugs.gnu.org/58531#19 it was suggested that
> eudc-plist-member, eudc-plist-get, and eudc-lax-plist-get could signal a
> more informative wrong-type-argument instead of a generic static error
> when passed a degenerate plist argument.  The attached patch makes this
> so.
>
> An alternative is to simply call plist-member and plist-get directly,
> relying on them for any error reporting, and not worry about arguments
> that degenerate further down the list than the point of interest.
>
> WDYT?  How important is it to name and shame degenerate plists as
> eagerly as possible in EUDC?

This patch looks good to me.  I haven't tested it, but if you have, feel
free to push to master.  (EUDC is not packaged in GNU ELPA, so it's OK
to use new functions like plistp without providing backward
compatibility.)

Thanks!
Thomas

bug#58720: 29.0.50; Improve error reporting of EUDC plist functions

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 58720 29.1
quit

Thomas Fitzsimmons [2022-10-22 14:06 -0400] wrote:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> In https://bugs.gnu.org/58531#19 it was suggested that
>> eudc-plist-member, eudc-plist-get, and eudc-lax-plist-get could signal a
>> more informative wrong-type-argument instead of a generic static error
>> when passed a degenerate plist argument.  The attached patch makes this
>> so.
>>
>> An alternative is to simply call plist-member and plist-get directly,
>> relying on them for any error reporting, and not worry about arguments
>> that degenerate further down the list than the point of interest.
>>
>> WDYT?  How important is it to name and shame degenerate plists as
>> eagerly as possible in EUDC?
>
> This patch looks good to me.  I haven't tested it, but if you have, feel
> free to push to master.  (EUDC is not packaged in GNU ELPA, so it's OK
> to use new functions like plistp without providing backward
> compatibility.)

Thanks.  Pushed and closing.

Improve error reporting of EUDC plist functions
9db7b11cf7 2022-10-23 00:44:52 +0300
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=9db7b11cf7

-- 
Basil