Comparing hash table objects

Comparing hash table objects

[Ihor Radchenko]

Hello,

Is there any way to compare objects containing hash tables for equality?

I have a struct that is created like
(list 'type (make-hash-table))
With hash table later populated with some key:value pairs.

I expect two structs like the above to be comparable via equal:
(equal (list 'type (make-hash-table)) (list 'type (make-hash-table))) ;; => t

However, running the above code gives nil.

Is the above behavior intentional?

Best,
Ihor

Re: Comparing hash table objects

[Andreas Schwab]

On Jun 08 2022, Ihor Radchenko wrote:

> Is there any way to compare objects containing hash tables for equality?

Hash tables are only equal if eq.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Comparing hash table objects

[Ihor Radchenko]

Andreas Schwab <schwab@suse.de> writes:

> On Jun 08 2022, Ihor Radchenko wrote:
>
>> Is there any way to compare objects containing hash tables for equality?
>
> Hash tables are only equal if eq.

This is somewhat unexpected.

To clarify my problem, let me explain in more details what I am trying
to achieve.

Currently, I am using structs to store objects with properties:
(setq obj1 (type (:key1 val1 :key2 val2 ...)))
(setq obj2 (type (:key1 val1 :key2 val2 ...))) ;; the same properties

With this approach, I can easily compare objects with (equal obj1 obj2),
use them as hash table keys, etc.

However, when the number of properties increases, :key access becomes
slow because plist-get has O(N) complexity.

The natural solution is using hash tables:

(setq obj1 (type (make-hash-table)))
(setq obj2 (type (make-hash-table)))
;; fill the hash tables with :keyN valN pairs

Now, I can retrieve the :key in O(1).

However, all of a sudden, I am unable to compare two objects with this
new internal structure. (equal obj1 obj2) returns nil.

The described behavior potentially generates a lot of room for errors.
It is relatively easy to remember that different hash table objects are
never equal (only equal when also eq). However, when the hash tables are
hidden inside a struct, it is easy to overlook that the structs cannot
be compared with equal as usual.

Best,
Ihor

Re: Comparing hash table objects

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

It clearly can be useful to compare hash tables for equivalence
of contents.  I see two ways to offer that facility:

* As a new function.  That would be upward-compatible.

* By making `equal' compare them that way.  That would fit the spirit
of `equal', but could break some existing uses of `equal'.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Comparing hash table objects

[Ihor Radchenko]

Richard Stallman <rms@gnu.org> writes:

> It clearly can be useful to compare hash tables for equivalence
> of contents.  I see two ways to offer that facility:
>
> * As a new function.  That would be upward-compatible.

I have seen various variants of such function in third-party packages
and just in internet search results.
For example, ht-equal? from
https://github.com/Wilfred/ht.el/blob/master/ht.el:

(defun ht-equal? (table1 table2)
  "Return t if TABLE1 and TABLE2 have the same keys and values.
Does not compare equality predicates."
  (declare (side-effect-free t))
  (let ((keys1 (ht-keys table1))
        (keys2 (ht-keys table2))
        (sentinel (make-symbol "ht-sentinel")))
    (and (equal (length keys1) (length keys2))
         (--all?
          (equal (ht-get table1 it)
                 (ht-get table2 it sentinel))
          keys1))))

However, it will not help with the problem of comparing objects
containing hash tables. Unless those obejcts also define special
comparison function (which is inconvenient).

> * By making `equal' compare them that way.  That would fit the spirit
> of `equal', but could break some existing uses of `equal'.

I'd say that using `equal' on hash tables with the actual intention of
comparing tables by `eq' is calling for a trouble. At least, it is
misleading.

Best,
Ihor

Re: Comparing hash table objects

[Stefan Monnier]

> However, it will not help with the problem of comparing objects
> containing hash tables. Unless those obejcts also define special
> comparison function (which is inconvenient).

This is an instance of a fairly general problem with Lisp's equality
tests (and it's not really specific to Lisp, admittedly).

Maybe a half-sane way to solve this problem is to provide a generic
"equality driver" which takes an argument specifying which objects to
compare for structural equality (i.e. where to keep recursing).


        Stefan

Re: Comparing hash table objects

[Ihor Radchenko]

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

>> However, it will not help with the problem of comparing objects
>> containing hash tables. Unless those obejcts also define special
>> comparison function (which is inconvenient).
>
> This is an instance of a fairly general problem with Lisp's equality
> tests (and it's not really specific to Lisp, admittedly).
>
> Maybe a half-sane way to solve this problem is to provide a generic
> "equality driver" which takes an argument specifying which objects to
> compare for structural equality (i.e. where to keep recursing).

This might be an option.
It would then help if cl-defstruct supported a default comparator
setting the "equality driver" to be used by default when comparing the
objects using equal.

Best,
Ihor

Re: Comparing hash table objects

[Sam Steingold]

> * Stefan Monnier <zbaavre@veb.hzbagerny.pn> [2022-06-11 12:04:46 -0400]:
>
>> However, it will not help with the problem of comparing objects
>> containing hash tables. Unless those obejcts also define special
>> comparison function (which is inconvenient).
>
> This is an instance of a fairly general problem with Lisp's equality
> tests (and it's not really specific to Lisp, admittedly).

Sadly, this is true.

> Maybe a half-sane way to solve this problem is to provide a generic
> "equality driver" which takes an argument specifying which objects to
> compare for structural equality (i.e. where to keep recursing).

The way Python does it could be a good starting point:
unless a class defines __eq__ method, the comparison is Lisp eq.
So, if a class has structure, it should define the comparison method.
See https://docs.python.org/3/reference/datamodel.html#object.__eq__

-- 
Sam Steingold (http://sds.podval.org/) on Pop 22.04 (jammy) X 11.0.12101003
http://childpsy.net http://calmchildstories.com http://steingoldpsychology.com
https://ij.org/ http://think-israel.org https://fairforall.org
(let ((a "(let ((a %c%s%c)) (format a 34 a 34))")) (format a 34 a 34))

Re: Comparing hash table objects

[Stefan Monnier]

Sam Steingold [2022-06-12 19:55:08] wrote:
> The way Python does it could be a good starting point:
> unless a class defines __eq__ method, the comparison is Lisp eq.
> So, if a class has structure, it should define the comparison method.
> See https://docs.python.org/3/reference/datamodel.html#object.__eq__

Seems like the exact same way ELisp works.  The problem we have right
now (translated into Python) is that some people now want to add
a `__eq__` method to the class `hash-table` (they argue that the lack of
it was an oversight).  It's easy to do what they ask for, but it breaks
backward compatibility.


        Stefan

Re: Comparing hash table objects

[Sam Steingold]

> * Stefan Monnier <zbaavre@veb.hzbagerny.pn> [2022-06-13 09:02:41 -0400]:
>
> Sam Steingold [2022-06-12 19:55:08] wrote:
>> The way Python does it could be a good starting point:
>> unless a class defines __eq__ method, the comparison is Lisp eq.
>> So, if a class has structure, it should define the comparison method.
>> See https://docs.python.org/3/reference/datamodel.html#object.__eq__
>
> Seems like the exact same way ELisp works.

including for user-defined classes?!

> The problem we have right now (translated into Python) is that some

please use ELisp terminology too for those who will want to read the sources.

> people now want to add a `__eq__` method to the class `hash-table`
> (they argue that the lack of it was an oversight).

sounds about right.

> It's easy to do what they ask for, but it breaks backward
> compatibility.

I would argue that code relying on `equal' being the same as `eq' for
`hash-table' is a bug.  Exposing such a bug might be painful, but it
should be fixed anyway.

Thank you for your clarifications.

-- 
Sam Steingold (https://youtube.com/channel/UCiW5WB6K4Fx-NhhtZscW7Og) on darwin Ns 10.3.2113
http://childpsy.net http://calmchildstories.com http://steingoldpsychology.com
https://ij.org/ https://ffii.org https://memri.org https://jij.org
He who laughs last thinks slowest.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hello!

persist-defvar does not persist records or hash tables correctly.

Here's a minimal example with a hash table:

(progn
  (persist-defvar foo (make-hash-table :test #'equal) "docstring")
  (puthash 'bar t foo)
  (persist-save 'foo)
  (gethash 'bar (persist-default 'foo))) ;; Expected nil, got t

This patch fixes persisting records by using copy-tree. Currently,
copy-tree does not work with records (see
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
The following snippet will return the expected value only when both this
patch and the above-linked patch are applied:

(progn
  (cl-defstruct baz a)
  (persist-defvar quux (make-baz :a nil) "docstring")
  (setf (baz-a quux) t)
  (persist-save 'quux)
  (baz-a (persist-default 'quux))) ;; Expected nil, got t

Before applying this patch, the updated values in both cases are not
persisted to disk. With the patch, the updated values are persisted as
expected.

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-persist-defvar-work-with-records-and-hash-table.patch --]
[-- Type: text/x-diff, Size: 1141 bytes --]

From b1512983fb82b9800ab6d0d1c9ca359e1251e94a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 14 May 2023 22:32:16 -0700
Subject: [PATCH] Make persist-defvar work with records and hash tables

Previously, when persist-defvar received a record or hash table for an
initial value, updated values were not persisted. This was because the
value and default value shared the same structure.
---
 persist.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index d80943d19e..a40fc2b297 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (eq 'hash-table (type-of initvalue))
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default (copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
-- 
2.40.1

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> Cc: Adam Porter <adam@alphapapa.net>
> Date: Sun, 14 May 2023 22:56:20 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> persist-defvar does not persist records or hash tables correctly.
> 
> Here's a minimal example with a hash table:
> 
> (progn
>   (persist-defvar foo (make-hash-table :test #'equal) "docstring")
>   (puthash 'bar t foo)
>   (persist-save 'foo)
>   (gethash 'bar (persist-default 'foo))) ;; Expected nil, got t
> 
> This patch fixes persisting records by using copy-tree. Currently,
> copy-tree does not work with records (see
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-05/msg00875.html>).
> The following snippet will return the expected value only when both this
> patch and the above-linked patch are applied:
> 
> (progn
>   (cl-defstruct baz a)
>   (persist-defvar quux (make-baz :a nil) "docstring")
>   (setf (baz-a quux) t)
>   (persist-save 'quux)
>   (baz-a (persist-default 'quux))) ;; Expected nil, got t
> 
> Before applying this patch, the updated values in both cases are not
> persisted to disk. With the patch, the updated values are persisted as
> expected.

Philip, any comments?

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

The patch should now work on Emacs versions before Emacs 30. I also
added tests for persisting records and hash tables.

Instead of copying the updated behavior of copy-tree into persist.el,
would it be more appropriate to require compat.el?

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]

From 7907521e72e2e99b883912f250b7afb14cbf5e80 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..0a85b78767 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-hash-equal.patch --]
[-- Type: text/x-diff, Size: 1460 bytes --]

From 41f90ac59a26018382d1bb9153af08ce4b9423ff Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:52:24 -0700
Subject: [PATCH 2/5] Add persist-hash-equal

See bug#63671.
---
 persist.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..0069273ca2 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,22 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-hash-equal (hash1 hash2)
+  "Return non-nil when the contents of HASH1 and HASH2 are equal.
+Table values are compared using `equal' unless they are both hash
+tables themselves, in which case `hash-equal' is used.
+Does not compare equality predicates."
+  (and (= (hash-table-count hash1)
+          (hash-table-count hash2))
+       (catch 'flag (maphash (lambda (key hash1-value)
+                               (let ((hash2-value (gethash key hash2)))
+                                 (or (if (and (hash-table-p hash1-value)
+                                              (hash-table-p hash2-value))
+                                         (hash-equal hash1-value hash2-value)
+                                       (equal hash1-value hash2-value))
+                                     (throw 'flag nil))))
+                             hash1)
+              t)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2988 bytes --]

From 53e166cb0c7c2624a5a54edafe8828d7b7edc612 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:09:29 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.
---
 persist.el            | 16 +++++++++++-----
 test/persist-tests.el |  8 ++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/persist.el b/persist.el
index 0069273ca2..725f2f71cf 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -132,9 +134,13 @@ (defun persist-save (symbol)
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+  (let ((symbol-file-loc (persist--file-location symbol))
+        (value (symbol-value symbol))
+        (default (persist-default symbol)))
+    (if (if (and (hash-table-p value)
+                 (hash-table-p default))
+            (persist-hash-equal value default)
+          (equal value default))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
@@ -148,7 +154,7 @@ (defun persist-save (symbol)
                 (print-escape-control-characters t)
                 (print-escape-nonascii t)
                 (print-circle t))
-            (print (symbol-value symbol) (current-buffer)))
+            (print value (current-buffer)))
           (write-region (point-min) (point-max)
                         symbol-file-loc
                         nil 'quiet))))))
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 0a85b78767..8a30a24e23 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (test-persist-save hash default
+                       (puthash 'foo "bar" (symbol-value sym))
+                       "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1912 bytes --]

From ccc1be590b6c7b71aaa4ee3dd3bf25184ea7c122 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.
---
 persist.el | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/persist.el b/persist.el
index 725f2f71cf..eb408b5dca 100644
--- a/persist.el
+++ b/persist.el
@@ -210,5 +210,34 @@ (defun persist-hash-equal (hash1 hash2)
                              hash1)
               t)))
 
+
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1763 bytes --]

From 4658bde147253d2f070b14c6f54300f640da063e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index eb408b5dca..39ce54bbf0 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 8a30a24e23..18b8af2b89 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                        (puthash 'foo "bar" (symbol-value sym))
                        "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (test-persist-save rec default
+                       (setf (aref (symbol-value sym) 2) 'quux)
+                       "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.40.1

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

I fixed the definition of persist-hash-equal, and rebased.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]

From 6ec3955a521266e0661bbd0b2a6d1984875ae1a1 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..0a85b78767 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-hash-equal.patch --]
[-- Type: text/x-diff, Size: 1475 bytes --]

From 1f6fae3a9799dbb58b742095480917c2e3b99a7f Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-hash-equal

See bug#63671.
---
 persist.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..fd0d750161 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,22 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-hash-equal (hash1 hash2)
+  "Return non-nil when the contents of HASH1 and HASH2 are equal.
+Table values are compared using `equal' unless they are both hash
+tables themselves, in which case `persist-hash-equal' is used.
+Does not compare equality predicates."
+  (and (= (hash-table-count hash1)
+          (hash-table-count hash2))
+       (catch 'flag (maphash (lambda (key hash1-value)
+                               (let ((hash2-value (gethash key hash2)))
+                                 (or (if (and (hash-table-p hash1-value)
+                                              (hash-table-p hash2-value))
+                                         (persist-hash-equal hash1-value hash2-value)
+                                       (equal hash1-value hash2-value))
+                                     (throw 'flag nil))))
+                             hash1)
+              t)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 3100 bytes --]

From ae35177b30bf1fabbc56ebf315b25a6074102201 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:09:29 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.
---
 persist.el            | 17 ++++++++++++-----
 test/persist-tests.el |  8 ++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/persist.el b/persist.el
index fd0d750161..9a0ee33a65 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -132,9 +134,14 @@ (defun persist-save (symbol)
   (unless (persist--persistant-p symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
-  (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+  (let* ((symbol-file-loc (persist--file-location symbol))
+         (value (symbol-value symbol))
+         (default (persist-default symbol))
+         (value-unchanged-p (if (and (hash-table-p value)
+                                     (hash-table-p default))
+                                (persist-hash-equal value default)
+                              (equal value default))))
+    (if value-unchanged-p
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
@@ -148,7 +155,7 @@ (defun persist-save (symbol)
                 (print-escape-control-characters t)
                 (print-escape-nonascii t)
                 (print-circle t))
-            (print (symbol-value symbol) (current-buffer)))
+            (print value (current-buffer)))
           (write-region (point-min) (point-max)
                         symbol-file-loc
                         nil 'quiet))))))
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 0a85b78767..8a30a24e23 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (test-persist-save hash default
+                       (puthash 'foo "bar" (symbol-value sym))
+                       "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1909 bytes --]

From 1b06fc7efb7339c390170e2bc26e298153221f2e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 9a0ee33a65..8eb0e7ba51 100644
--- a/persist.el
+++ b/persist.el
@@ -211,5 +211,33 @@ (defun persist-hash-equal (hash1 hash2)
                              hash1)
               t)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 1763 bytes --]

From afd6ea405efb1cba0ad1b1601f4af8efc796c1d7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 8eb0e7ba51..2f3885f80e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 8a30a24e23..18b8af2b89 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                        (puthash 'foo "bar" (symbol-value sym))
                        "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (test-persist-save rec default
+                       (setf (aref (symbol-value sym) 2) 'quux)
+                       "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 02 Sep 2023 16:54:00 -0700
> 
> I fixed the definition of persist-hash-equal, and rebased.

Thanks, but this lacks the commit log messages (see CONTRIBUTE).

Also, I think the new features warrant a NEWS entry.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but this lacks the commit log messages (see CONTRIBUTE).

I have added commit log messages. See attached patches.

> Also, I think the new features warrant a NEWS entry.

Should that go in the NEWS file in the main Emacs repo? I don't see a
NEWS file in either of the main or externals/persist ELPA branches.

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3629 bytes --]

From 2ca778a44c10f11059f16ef5922cf1eff9118105 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

* test/persist-tests.el (persist-test-persist-save):
Consolidate logic of test-persist-save and
test-persist-save-non-number.
(test-persist-save-number test-persist-save-string): Simple wrappers
over persist-test-persist-save
---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..f2b04769ef 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro persist-test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (persist-test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-equal.patch --]
[-- Type: text/x-diff, Size: 1394 bytes --]

From b379d8d1779e0190541ef7f8adf39dfe4d4c551a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-equal

* persist.el (persist-hash-equal): Like equal, but compares hash
tables also.
See bug#63671 for a discussion of built-in hash table comparison.
---
 persist.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..a707d038cd 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,25 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-equal (a b)
+  "Return non-nil when the values of A and B are equal.
+A and B are compared using `equal' unless they are both hash
+tables. In that case, the following are compared:
+
+- hash table count
+- hash table predicate
+- values, using `persist-equal'"
+  (if (and (hash-table-p a) (hash-table-p b))
+      (and (= (hash-table-count a) (hash-table-count b))
+           (eq (hash-table-test a) (hash-table-test b))
+           (catch 'done
+             (maphash
+              (lambda (key a-value)
+                (unless (persist-equal a-value (gethash key b (not a-value)))
+                  (throw 'done nil)))
+              a)
+             t))
+    (equal a b)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]

From 11194569423dcdf8778a55f59dbca8f49e8b7b37 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 3 Sep 2023 17:10:38 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Use hash table copy as default so that
the original table can be modified without modifying the default value.
(persist-save): Use persist-equal to ensure that hash tables are
correctly compared
* test/persist-tests.el (test-persist-save-hash): Test hash tables
persistence.
---
 persist.el            | 8 +++++---
 test/persist-tests.el | 8 ++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/persist.el b/persist.el
index a707d038cd..7b2ab491d7 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -133,8 +135,8 @@ (defun persist-save (symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
   (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+    (if (persist-equal (symbol-value symbol)
+                       (persist-default symbol))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
diff --git a/test/persist-tests.el b/test/persist-tests.el
index f2b04769ef..90adf1c6d6 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (persist-test-persist-save hash default
+                               (puthash 'foo "bar" (symbol-value sym))
+                               "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1983 bytes --]

From f360f7ae53125a847c2a8d5762ca5f08d16445b9 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.

* persist.el (persist-copy-tree): Add copy-tree, so that records can
be compared.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 7b2ab491d7..93444995f2 100644
--- a/persist.el
+++ b/persist.el
@@ -209,5 +209,33 @@ (defun persist-equal (a b)
              t))
     (equal a b)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (persist-copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (persist-copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (persist-copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 2019 bytes --]

From ea57e6205b10678b9c26f7dcf3704e2a7acb25a7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Set default to a copy of initvalue so
when initvalue is a record, the original can be modified without
modifying the default.
* test/persist-tests.el: Test persist-save with a record.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 93444995f2..e43171459e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 90adf1c6d6..62d8501493 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                                (puthash 'foo "bar" (symbol-value sym))
                                "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (persist-test-persist-save rec default
+                               (setf (aref (symbol-value sym) 2) 'quux)
+                               "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
> Date: Sun, 03 Sep 2023 17:29:22 -0700
> 
> > Also, I think the new features warrant a NEWS entry.
> 
> Should that go in the NEWS file in the main Emacs repo? I don't see a
> NEWS file in either of the main or externals/persist ELPA branches.

I'm terribly sorry: I haven't realized this is for ELPA, I thought it
was for emacs.git.  My bad Please ignore what I wrote in my previous
message.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>> 
>> > Also, I think the new features warrant a NEWS entry.
>> 
>> Should that go in the NEWS file in the main Emacs repo? I don't see a
>> NEWS file in either of the main or externals/persist ELPA branches.
>
>I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>was for emacs.git.  My bad Please ignore what I wrote in my previous
>message.

No problem! It gave me a chance to do a final editing pass. What's the next step? I would be glad for Phillip's review, if possible.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Joseph Turner [2023-09-04 08:57:13] wrote:
> On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>>> 
>>> > Also, I think the new features warrant a NEWS entry.
>>> 
>>> Should that go in the NEWS file in the main Emacs repo? I don't see a
>>> NEWS file in either of the main or externals/persist ELPA branches.
>>
>>I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>>was for emacs.git.  My bad Please ignore what I wrote in my previous
>>message.
>
> No problem! It gave me a chance to do a final editing pass. What's the next
> step? I would be glad for Phillip's review, if possible.

I was about to ping Phil over on https://gitlab.com/phillord/persist/
but was reminded along the way that Phil gave me write access to
that repository.

Could you send me the result of your final editing pass?


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Stefan Monnier <monnier@iro.umontreal.ca> writes:
> I was about to ping Phil over on https://gitlab.com/phillord/persist/
> but was reminded along the way that Phil gave me write access to
> that repository.
>
> Could you send me the result of your final editing pass?

Here you go!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-persist-save-macro.patch --]
[-- Type: text/x-diff, Size: 3629 bytes --]

From 2ca778a44c10f11059f16ef5922cf1eff9118105 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 12:57:02 -0700
Subject: [PATCH 1/5] Add test-persist-save macro

* test/persist-tests.el (persist-test-persist-save):
Consolidate logic of test-persist-save and
test-persist-save-non-number.
(test-persist-save-number test-persist-save-string): Simple wrappers
over persist-test-persist-save
---
 test/persist-tests.el | 76 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 43 deletions(-)

diff --git a/test/persist-tests.el b/test/persist-tests.el
index b6645a9297..f2b04769ef 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -25,51 +25,41 @@ (ert-deftest test-persist-save-only-persistant ()
    :type 'error
    :exclude-subtypes t))
 
-(ert-deftest test-persist-save ()
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     ;; precondition
-   (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 10)
-     (persist-symbol sym 10)
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym 20)
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "20"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string))))
-     (set sym 10)
-     (persist-save sym)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (should-error
-      (persist-save 'fred)))))
+(defmacro persist-test-persist-save (init default change printed-changed)
+  "Test persisting symbols.
+- symbol is not persisted when value is set to INIT and default
+  value is set to DEFAULT.
+- symbol is persisted when value is changed according to CHANGE.
+- persisted file contents match PRINTED-CHANGED.
+- symbol is not persisted after value is set back to DEFAULT."
+  `(with-local-temp-persist
+    (let ((sym (cl-gensym)))
+      (should-not (file-exists-p (persist--file-location sym)))
+      (set sym ,init)
+      (persist-symbol sym ,default)
+      (persist-save sym)
+      (should t)
+      (should-not (file-exists-p (persist--file-location sym)))
+      ,change
+      (persist-save sym)
+      (should (file-exists-p (persist--file-location sym)))
+      (should
+       (string-match-p
+        ,printed-changed
+        (with-temp-buffer
+          (insert-file-contents (persist--file-location sym))
+          (buffer-string))))
+      (set sym ,default)
+      (persist-save sym)
+      (should-not (file-exists-p (persist--file-location sym))))))
 
-(ert-deftest test-persist-save-non-number ()
-  "Test saving something that is not a number.
+(ert-deftest test-persist-save-number ()
+  "Test saving number."
+  (persist-test-persist-save 1 1 (set sym 2) "2"))
 
-`test-persist-save' missed "
-  (with-local-temp-persist
-   (let ((sym (cl-gensym)))
-     (set sym "fred")
-     (persist-symbol sym "fred")
-     (persist-save sym)
-     (should t)
-     (should-not (file-exists-p (persist--file-location sym)))
-     (set sym "george")
-     (persist-save sym)
-     (should (file-exists-p (persist--file-location sym)))
-     (should
-      (string-match-p
-       "george"
-       (with-temp-buffer
-         (insert-file-contents (persist--file-location sym))
-         (buffer-string)))))))
+(ert-deftest test-persist-save-string ()
+  "Test saving string."
+  (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-persist-equal.patch --]
[-- Type: text/x-diff, Size: 1394 bytes --]

From b379d8d1779e0190541ef7f8adf39dfe4d4c551a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 2 Sep 2023 16:52:31 -0700
Subject: [PATCH 2/5] Add persist-equal

* persist.el (persist-hash-equal): Like equal, but compares hash
tables also.
See bug#63671 for a discussion of built-in hash table comparison.
---
 persist.el | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/persist.el b/persist.el
index d80943d19e..a707d038cd 100644
--- a/persist.el
+++ b/persist.el
@@ -187,5 +187,25 @@ (defun persist--save-all ()
 (add-hook 'kill-emacs-hook
           'persist--save-all)
 
+(defun persist-equal (a b)
+  "Return non-nil when the values of A and B are equal.
+A and B are compared using `equal' unless they are both hash
+tables. In that case, the following are compared:
+
+- hash table count
+- hash table predicate
+- values, using `persist-equal'"
+  (if (and (hash-table-p a) (hash-table-p b))
+      (and (= (hash-table-count a) (hash-table-count b))
+           (eq (hash-table-test a) (hash-table-test b))
+           (catch 'done
+             (maphash
+              (lambda (key a-value)
+                (unless (persist-equal a-value (gethash key b (not a-value)))
+                  (throw 'done nil)))
+              a)
+             t))
+    (equal a b)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-persist-defvar-work-with-hash-tables.patch --]
[-- Type: text/x-diff, Size: 2645 bytes --]

From 11194569423dcdf8778a55f59dbca8f49e8b7b37 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 3 Sep 2023 17:10:38 -0700
Subject: [PATCH 3/5] Make persist-defvar work with hash tables

Previously, when persist-defvar received a hash table for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Use hash table copy as default so that
the original table can be modified without modifying the default value.
(persist-save): Use persist-equal to ensure that hash tables are
correctly compared
* test/persist-tests.el (test-persist-save-hash): Test hash tables
persistence.
---
 persist.el            | 8 +++++---
 test/persist-tests.el | 8 ++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/persist.el b/persist.el
index a707d038cd..7b2ab491d7 100644
--- a/persist.el
+++ b/persist.el
@@ -118,7 +118,9 @@ (defun persist-symbol (symbol &optional initvalue)
   (let ((initvalue (or initvalue (symbol-value symbol))))
     (add-to-list 'persist--symbols symbol)
     (put symbol 'persist t)
-    (put symbol 'persist-default initvalue)))
+    (if (hash-table-p initvalue)
+        (put symbol 'persist-default (copy-hash-table initvalue))
+      (put symbol 'persist-default initvalue))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
@@ -133,8 +135,8 @@ (defun persist-save (symbol)
     (error (format
             "Symbol %s is not persistant" symbol)))
   (let ((symbol-file-loc (persist--file-location symbol)))
-    (if (equal (symbol-value symbol)
-               (persist-default symbol))
+    (if (persist-equal (symbol-value symbol)
+                       (persist-default symbol))
         (when (file-exists-p symbol-file-loc)
           (delete-file symbol-file-loc))
       (let ((dir-loc
diff --git a/test/persist-tests.el b/test/persist-tests.el
index f2b04769ef..90adf1c6d6 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -61,6 +61,14 @@ (ert-deftest test-persist-save-string ()
   "Test saving string."
   (persist-test-persist-save "foo" "foo" (set sym "bar") "bar"))
 
+(ert-deftest test-persist-save-hash ()
+  "Test saving hash table."
+  (let* ((hash (make-hash-table))
+         (default (copy-hash-table hash)))
+    (persist-test-persist-save hash default
+                               (puthash 'foo "bar" (symbol-value sym))
+                               "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Add-persist-copy-tree.patch --]
[-- Type: text/x-diff, Size: 1983 bytes --]

From f360f7ae53125a847c2a8d5762ca5f08d16445b9 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:43:02 -0700
Subject: [PATCH 4/5] Add persist-copy-tree

The behavior of copy-tree was changed in Emacs 30. This function will
ensure that persist works correctly for previous Emacs versions.

* persist.el (persist-copy-tree): Add copy-tree, so that records can
be compared.
---
 persist.el | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/persist.el b/persist.el
index 7b2ab491d7..93444995f2 100644
--- a/persist.el
+++ b/persist.el
@@ -209,5 +209,33 @@ (defun persist-equal (a b)
              t))
     (equal a b)))
 
+(defun persist-copy-tree (tree &optional vectors-and-records)
+  "Make a copy of TREE.
+If TREE is a cons cell, this recursively copies both its car and its cdr.
+Contrast to `copy-sequence', which copies only along the cdrs.
+With the second argument VECTORS-AND-RECORDS non-nil, this
+traverses and copies vectors and records as well as conses."
+  (declare (side-effect-free error-free))
+  (if (consp tree)
+      (let (result)
+	(while (consp tree)
+	  (let ((newcar (car tree)))
+	    (if (or (consp (car tree))
+                    (and vectors-and-records
+                         (or (vectorp (car tree)) (recordp (car tree)))))
+		(setq newcar (persist-copy-tree (car tree) vectors-and-records)))
+	    (push newcar result))
+	  (setq tree (cdr tree)))
+	(nconc (nreverse result)
+               (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+                   (persist-copy-tree tree vectors-and-records)
+                 tree)))
+    (if (and vectors-and-records (or (vectorp tree) (recordp tree)))
+	(let ((i (length (setq tree (copy-sequence tree)))))
+	  (while (>= (setq i (1- i)) 0)
+	    (aset tree i (persist-copy-tree (aref tree i) vectors-and-records)))
+	  tree)
+      tree)))
+
 (provide 'persist)
 ;;; persist.el ends here
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Make-persist-defvar-work-with-records.patch --]
[-- Type: text/x-diff, Size: 2019 bytes --]

From ea57e6205b10678b9c26f7dcf3704e2a7acb25a7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 23 May 2023 13:44:40 -0700
Subject: [PATCH 5/5] Make persist-defvar work with records

Previously, when persist-defvar received a record for an initial
value, updated values were not persisted.

* persist.el (persist-symbol): Set default to a copy of initvalue so
when initvalue is a record, the original can be modified without
modifying the default.
* test/persist-tests.el: Test persist-save with a record.
---
 persist.el            | 2 +-
 test/persist-tests.el | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/persist.el b/persist.el
index 93444995f2..e43171459e 100644
--- a/persist.el
+++ b/persist.el
@@ -120,7 +120,7 @@ (defun persist-symbol (symbol &optional initvalue)
     (put symbol 'persist t)
     (if (hash-table-p initvalue)
         (put symbol 'persist-default (copy-hash-table initvalue))
-      (put symbol 'persist-default initvalue))))
+      (put symbol 'persist-default (persist-copy-tree initvalue t)))))
 
 (defun persist--persistant-p (symbol)
   "Return non-nil if SYMBOL is a persistant variable."
diff --git a/test/persist-tests.el b/test/persist-tests.el
index 90adf1c6d6..62d8501493 100644
--- a/test/persist-tests.el
+++ b/test/persist-tests.el
@@ -69,6 +69,14 @@ (ert-deftest test-persist-save-hash ()
                                (puthash 'foo "bar" (symbol-value sym))
                                "#s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data (foo \"bar\"))")))
 
+(ert-deftest test-persist-save-record ()
+  "Test saving record."
+  (let* ((rec (record 'foo 'a 'b))
+         (default (copy-sequence rec)))
+    (persist-test-persist-save rec default
+                               (setf (aref (symbol-value sym) 2) 'quux)
+                               "#s(foo a quux)")))
+
 (ert-deftest test-persist-load ()
   (with-local-temp-persist
    (let ((sym (cl-gensym)))
-- 
2.41.0

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[phillip.lord]

On 2023-09-04 18:05, Stefan Monnier wrote:
> Joseph Turner [2023-09-04 08:57:13] wrote:
>> On September 4, 2023 4:33:55 AM PDT, Eli Zaretskii <eliz@gnu.org> 
>> wrote:
>>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>>> Cc: phillip.lord@russet.org.uk, monnier@iro.umontreal.ca,
>>>>  63513@debbugs.gnu.org, Adam Porter <adam.porter@47ap.net>
>>>> Date: Sun, 03 Sep 2023 17:29:22 -0700
>>>> 
>>>> > Also, I think the new features warrant a NEWS entry.
>>>> 
>>>> Should that go in the NEWS file in the main Emacs repo? I don't see 
>>>> a
>>>> NEWS file in either of the main or externals/persist ELPA branches.
>>> 
>>> I'm terribly sorry: I haven't realized this is for ELPA, I thought it
>>> was for emacs.git.  My bad Please ignore what I wrote in my previous
>>> message.
>> 
>> No problem! It gave me a chance to do a final editing pass. What's the 
>> next
>> step? I would be glad for Phillip's review, if possible.
> 
> I was about to ping Phil over on https://gitlab.com/phillord/persist/
> but was reminded along the way that Phil gave me write access to
> that repository.
> 
> Could you send me the result of your final editing pass?


Indeed! I am afraid I am fairly unresponsive these days ("real life" 
intrudes), but I am more than happy for you to update there.

Phil

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Here you go!

Thanks, pushed!


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

>> Here you go!
>
> Thanks, pushed!

May it be possible to promote `persist-hash-equal' and
`persist-copy-tree' to common subr.el functions?

The topic of comparing hash tables has been previously discussed in
https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Joseph Turner <joseph@breatheoutbreathe.in>, Adam Porter
>  <adam@alphapapa.net>, Eli Zaretskii <eliz@gnu.org>,
>  phillip.lord@russet.org.uk, 63513-done@debbugs.gnu.org
> Date: Fri, 08 Sep 2023 11:30:52 +0000
> 
> May it be possible to promote `persist-hash-equal' and
> `persist-copy-tree' to common subr.el functions?

Why do we need them in subr.el, i.e. preloaded?  Why cannot these
functions be autoloaded instead?

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> May it be possible to promote `persist-hash-equal' and
>> `persist-copy-tree' to common subr.el functions?
>
> Why do we need them in subr.el, i.e. preloaded?  Why cannot these
> functions be autoloaded instead?

Similar functions - `string-equal-ignore-case' and `copy-tree' - are
already in subr.el. I do not see why the new functions discussed here
should be any different.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: monnier@iro.umontreal.ca, joseph@breatheoutbreathe.in,
>  adam@alphapapa.net, phillip.lord@russet.org.uk, 63513-done@debbugs.gnu.org
> Date: Fri, 08 Sep 2023 12:06:06 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> May it be possible to promote `persist-hash-equal' and
> >> `persist-copy-tree' to common subr.el functions?
> >
> > Why do we need them in subr.el, i.e. preloaded?  Why cannot these
> > functions be autoloaded instead?
> 
> Similar functions - `string-equal-ignore-case' and `copy-tree' - are
> already in subr.el. I do not see why the new functions discussed here
> should be any different.

We make the decision whether a function must be preloaded in a case by
case basis, to avoid making the memory footprint of an Emacs session
larger than it needs to be.  So arguments "by similarity" are not
useful in this case; you need to explain why you think these functions
are needed right from the startup.  Valid reasons include, among
others:

  . function is used by the startup code or during dumping
  . function is used by all or many important configurations
    immediately after startup

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Why do we need them in subr.el, i.e. preloaded?  Why cannot these
>> > functions be autoloaded instead?
>> 
>> Similar functions - `string-equal-ignore-case' and `copy-tree' - are
>> already in subr.el. I do not see why the new functions discussed here
>> should be any different.
>
> We make the decision whether a function must be preloaded in a case by
> case basis, to avoid making the memory footprint of an Emacs session
> larger than it needs to be.  So arguments "by similarity" are not
> useful in this case; you need to explain why you think these functions
> are needed right from the startup.
> ...

Got it. Then, I do not insist on subr.el specifically. Autoloading is
fine.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> Here you go!
>> Thanks, pushed!
> May it be possible to promote `persist-hash-equal' and
> `persist-copy-tree' to common subr.el functions?
> The topic of comparing hash tables has been previously discussed in
> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/

Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
so there's nothing to do there :-)

As for `persist-hash-equal` (which is actually called `persist-equal`,
AFAICT), I think if we want to move it out of `persist.el` we need to
improve it so it also looks inside hash-tables that occur within
vectors, conses, etc...

Maybe, like Sam Steingold suggested, we should simply consider the
current treatment of `equal` on hash-tables to be a bug.


        Stefan

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

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

>>>> Here you go!
>>> Thanks, pushed!
>> May it be possible to promote `persist-hash-equal' and
>> `persist-copy-tree' to common subr.el functions?
>> The topic of comparing hash tables has been previously discussed in
>> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/
>
> Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
> so there's nothing to do there :-)

Err...
Then, wouldn't it be better to contribute this function to compat.el and
use it from there?

> As for `persist-hash-equal` (which is actually called `persist-equal`,
> AFAICT), I think if we want to move it out of `persist.el` we need to
> improve it so it also looks inside hash-tables that occur within
> vectors, conses, etc...
>
> Maybe, like Sam Steingold suggested, we should simply consider the
> current treatment of `equal` on hash-tables to be a bug.

That thread had no progress for quite a while.
I thought that having `hash-equal' function could at least be an
improvement (clearly, it is being used 🙂).

But if you have an idea how to move the original discussion forward,
please share it. AFAIU, no actionable conclusion have been reached in
the linked thread.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ihor Radchenko <yantar92@posteo.net> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>>> Here you go!
>>>> Thanks, pushed!
>>> May it be possible to promote `persist-hash-equal' and
>>> `persist-copy-tree' to common subr.el functions?
>>> The topic of comparing hash tables has been previously discussed in
>>> https://yhetil.org/emacs-devel/871qvz4kdw.fsf@localhost/
>>
>> Hmm... AFAICT `persist-copy-tree` is a copy of our current `copy-tree`,
>> so there's nothing to do there :-)
>
> Err...
> Then, wouldn't it be better to contribute this function to compat.el and
> use it from there?

The new behavior of copy-tree has already been added to compat.el:

https://github.com/emacs-compat/compat/pull/25

However, that change currently only exists in compat's emacs-30 branch.

I did not know if it was acceptable for persist.el to require compat
when I wrote these patches. If we agree that it is acceptable, then I'm
happy to submit a patch to replace (persist-copy-tree ...) with
(compat-call copy-tree ...), but we'll have to wait to apply the patch
until after the compat.el emacs-30 branch is merged into master.

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

>> Then, wouldn't it be better to contribute this function to compat.el and
>> use it from there?
>
> The new behavior of copy-tree has already been added to compat.el:
>
> https://github.com/emacs-compat/compat/pull/25
>
> However, that change currently only exists in compat's emacs-30 branch.

I see.

> I did not know if it was acceptable for persist.el to require compat
> when I wrote these patches. If we agree that it is acceptable, then I'm
> happy to submit a patch to replace (persist-copy-tree ...) with
> (compat-call copy-tree ...), but we'll have to wait to apply the patch
> until after the compat.el emacs-30 branch is merged into master.

AFAIU, the recommended way to implement compat function definitions that
are not yet added to compat.el is using `compat-defun' + `compat-call'.
Then, one can simply drop `compat-defun' after the function is finally
release with compat.el without touching the rest of the code.

CCing Daniel.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Daniel Mendler]

On 9/9/23 12:01, Ihor Radchenko wrote:
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
> 
>>> Then, wouldn't it be better to contribute this function to compat.el and
>>> use it from there?
>>
>> The new behavior of copy-tree has already been added to compat.el:
>>
>> https://github.com/emacs-compat/compat/pull/25
>>
>> However, that change currently only exists in compat's emacs-30 branch.
> 
> I see.
>
>> I did not know if it was acceptable for persist.el to require compat
>> when I wrote these patches. If we agree that it is acceptable, then I'm
>> happy to submit a patch to replace (persist-copy-tree ...) with
>> (compat-call copy-tree ...), but we'll have to wait to apply the patch
>> until after the compat.el emacs-30 branch is merged into master.
> 
> AFAIU, the recommended way to implement compat function definitions that
> are not yet added to compat.el is using `compat-defun' + `compat-call'.
> Then, one can simply drop `compat-defun' after the function is finally
> release with compat.el without touching the rest of the code.

`compat-call` is meant to be used to call "extended functions", for
example functions with additional arguments. See the Compat manual for
details.

The macros from compat-macs.el (`compat-defun` etc.) are internal as
documented in the file compat-macs.el. These macros must not be used
outside Compat.

So using Compat here has to wait until compat-30.x is released.

Daniel

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Ihor Radchenko]

Daniel Mendler <mail@daniel-mendler.de> writes:

>> AFAIU, the recommended way to implement compat function definitions that
>> are not yet added to compat.el is using `compat-defun' + `compat-call'.
>> Then, one can simply drop `compat-defun' after the function is finally
>> release with compat.el without touching the rest of the code.
>
> `compat-call` is meant to be used to call "extended functions", for
> example functions with additional arguments. See the Compat manual for
> details.
>
> The macros from compat-macs.el (`compat-defun` etc.) are internal as
> documented in the file compat-macs.el. These macros must not be used
> outside Compat.
>
> So using Compat here has to wait until compat-30.x is released.

And do I understand correctly that compat-30 will only be released after
Emacs 30 is released? If so, it is awkward for :core packages.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#63513: [PATCH] Make persist-defvar work with records and hash tables

[Daniel Mendler]

On 9/9/23 13:35, Ihor Radchenko wrote:
>> So using Compat here has to wait until compat-30.x is released.
> 
> And do I understand correctly that compat-30 will only be released after
> Emacs 30 is released? If so, it is awkward for :core packages.

compat-30 can be released as soon as Emacs 30 has been reasonably
stabilized, e.g., when the emacs-30 branch has been frozen, or a bit
before that. We cannot release much earlier since APIs may still change
and it is probably undesired to release unfinished APIs to the public
too early. For reference, I've created the compat-29.1.1 release around
the day that Eli announced the emacs-29 branch freeze.

Daniel

compat.el and Emacs unstable master branch features (was: bug#63513: [PATCH] Make persist-defvar work with records and hash tables)

[Ihor Radchenko]

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 9/9/23 13:35, Ihor Radchenko wrote:
>>> So using Compat here has to wait until compat-30.x is released.
>> 
>> And do I understand correctly that compat-30 will only be released after
>> Emacs 30 is released? If so, it is awkward for :core packages.
>
> compat-30 can be released as soon as Emacs 30 has been reasonably
> stabilized, e.g., when the emacs-30 branch has been frozen, or a bit
> before that. We cannot release much earlier since APIs may still change
> and it is probably undesired to release unfinished APIs to the public
> too early. For reference, I've created the compat-29.1.1 release around
> the day that Eli announced the emacs-29 branch freeze.

I understand the logic, but it does not make things less awkward.

Sometimes, Emacs core packages that are also distributed on ELPA rely on
bugfixes (changed functions) or new functions from master.

It is indeed viable to copy-paste the needed functions into the package
code, but it _feels_ like something compat.el may provide support for.

May it be possible to provide a public API in compat.el to define
compat function versions before they appear in compat.el properly?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: compat.el and Emacs unstable master branch features (was: bug#63513: [PATCH] Make persist-defvar work with records and hash tables)

[Daniel Mendler]

On 9/9/23 14:12, Ihor Radchenko wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
>> compat-30 can be released as soon as Emacs 30 has been reasonably
>> stabilized, e.g., when the emacs-30 branch has been frozen, or a bit
>> before that. We cannot release much earlier since APIs may still change
>> and it is probably undesired to release unfinished APIs to the public
>> too early. For reference, I've created the compat-29.1.1 release around
>> the day that Eli announced the emacs-29 branch freeze.
>
> It is indeed viable to copy-paste the needed functions into the package
> code, but it _feels_ like something compat.el may provide support for.
>
> May it be possible to provide a public API in compat.el to define
> compat function versions before they appear in compat.el properly?

I see your point, but I think that Compat provides value for both
external and core packages given its current conservative approach. All
that is needed is to wait for a while after some API has been added and
Compat has been synchronized. Note that we haven't seen many API
additions in Emacs 30 so far. Compare this to compat-29.el which
provides a rich set of new APIs which are all available for external and
core packages depending on older Emacs versions.

I suggest to copy new functions temporarily to the package in question
with an fboundp check, with an additional TODO comment. We can
synchronize with Compat afterwards. Providing a public API won't work
since Compat is not included in Emacs itself. A design criterion of
Compat is also to keep the public API surface as small as possible.

Daniel

Re: compat.el and Emacs unstable master branch features (was: bug#63513: [PATCH] Make persist-defvar work with records and hash tables)

[Joseph Turner]

Daniel Mendler <mail@daniel-mendler.de> writes:

> I suggest to copy new functions temporarily to the package in question
> with an fboundp check, with an additional TODO comment. We can
> synchronize with Compat afterwards. Providing a public API won't work
> since Compat is not included in Emacs itself. A design criterion of
> Compat is also to keep the public API surface as small as possible.

fboundp won't work here because copy-tree is available in earlier
versions of Emacs. Since its behavior changed in 30, we could do a
version check + TODO comment?

Re: compat.el and Emacs unstable master branch features (was: bug#63513: [PATCH] Make persist-defvar work with records and hash tables)

[Ihor Radchenko]

Daniel Mendler <mail@daniel-mendler.de> writes:

> ... Providing a public API won't work
> since Compat is not included in Emacs itself. A design criterion of
> Compat is also to keep the public API surface as small as possible.

Maybe it is the time to consider including the compat.el API into Emacs?
We are getting a number of :core packages published on ELPA and
naturally having to solve various compatibility problems.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: compat.el and Emacs unstable master branch features

[Philip Kaludercic]

Ihor Radchenko <yantar92@posteo.net> writes:

> Daniel Mendler <mail@daniel-mendler.de> writes:
>
>> ... Providing a public API won't work
>> since Compat is not included in Emacs itself. A design criterion of
>> Compat is also to keep the public API surface as small as possible.
>
> Maybe it is the time to consider including the compat.el API into Emacs?
> We are getting a number of :core packages published on ELPA and
> naturally having to solve various compatibility problems.

I am a bit behind wrt Compat development.  Are we talking about adding
the `compat-call, etc. macros to the core?  So basically just a

  (defmacro compat-call (&rest args) args)

that would be overriden by compat's compat-call, as soon as that is
loaded

Re: compat.el and Emacs unstable master branch features

[Daniel Mendler]

On 9/12/23 12:02, Philip Kaludercic wrote:
> Ihor Radchenko <yantar92@posteo.net> writes:
> 
>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>
>>> ... Providing a public API won't work
>>> since Compat is not included in Emacs itself. A design criterion of
>>> Compat is also to keep the public API surface as small as possible.
>>
>> Maybe it is the time to consider including the compat.el API into Emacs?
>> We are getting a number of :core packages published on ELPA and
>> naturally having to solve various compatibility problems.
> 
> I am a bit behind wrt Compat development.  Are we talking about adding
> the `compat-call, etc. macros to the core?  So basically just a
> 
>   (defmacro compat-call (&rest args) args)
> 
> that would be overriden by compat's compat-call, as soon as that is
> loaded

Yes, if the Emacs maintainers agree I think this could be done. Take
compat.el, remove the part which requires compat-29, and copy it to
lisp/ or lisp/emacs-lisp/. As you said, if Compat (the package) is
installed it will be preferred over the core compat.el. The advantage of
this move is that core package could require compat directly without
passing a noerror argument to require. Furthermore `compat-call' and
`compat-function' would be available and wouldn't have to be copied as
is currently the case for example in erc-compat.el.

Daniel

Re: compat.el and Emacs unstable master branch features

[Philip Kaludercic]

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

Daniel Mendler <mail@daniel-mendler.de> writes:

> On 9/12/23 12:02, Philip Kaludercic wrote:
>> Ihor Radchenko <yantar92@posteo.net> writes:
>> 
>>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>>
>>>> ... Providing a public API won't work
>>>> since Compat is not included in Emacs itself. A design criterion of
>>>> Compat is also to keep the public API surface as small as possible.
>>>
>>> Maybe it is the time to consider including the compat.el API into Emacs?
>>> We are getting a number of :core packages published on ELPA and
>>> naturally having to solve various compatibility problems.
>> 
>> I am a bit behind wrt Compat development.  Are we talking about adding
>> the `compat-call, etc. macros to the core?  So basically just a
>> 
>>   (defmacro compat-call (&rest args) args)
>> 
>> that would be overriden by compat's compat-call, as soon as that is
>> loaded
>
> Yes, if the Emacs maintainers agree I think this could be done. Take
> compat.el, remove the part which requires compat-29, and copy it to
> lisp/ or lisp/emacs-lisp/. As you said, if Compat (the package) is
> installed it will be preferred over the core compat.el. The advantage of
> this move is that core package could require compat directly without
> passing a noerror argument to require. Furthermore `compat-call' and
> `compat-function' would be available and wouldn't have to be copied as
> is currently the case for example in erc-compat.el.

That sounds good!  How does this look like:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-the-public-API-of-Compat-to-the-core.patch --]
[-- Type: text/x-diff, Size: 3834 bytes --]

From 3245c3c4e6a8a4c35c4072df3a0bfefcabe0555d Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 13 Sep 2023 12:26:22 +0200
Subject: [PATCH] Add the public API of Compat to the core

* lisp/emacs-lisp/compat.el: Add stub file with minimal definitions,
so that core packages, that haven't been installed from ELPA, can make
use of the public API and use more recent function signatures.
* lisp/progmodes/python.el (compat): Remove 'noerror flag, because
Compat can now be required without the real package being available.
---
 lisp/emacs-lisp/compat.el | 61 +++++++++++++++++++++++++++++++++++++++
 lisp/progmodes/python.el  |  2 +-
 2 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 lisp/emacs-lisp/compat.el

diff --git a/lisp/emacs-lisp/compat.el b/lisp/emacs-lisp/compat.el
new file mode 100644
index 00000000000..4c60093b6be
--- /dev/null
+++ b/lisp/emacs-lisp/compat.el
@@ -0,0 +1,61 @@
+;;; compat.el --- Pseudo-Compatibility for Elisp -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2021-2023 Free Software Foundation, Inc.
+
+;; Author:								\
+;;   Philip Kaludercic <philipk@posteo.net>,				\
+;;   Daniel Mendler <mail@daniel-mendler.de>
+;; Maintainer:								\
+;;   Daniel Mendler <mail@daniel-mendler.de>,				\
+;;   Compat Development <~pkal/compat-devel@lists.sr.ht>,
+;;   emacs-devel@gnu.org
+;; URL: https://github.com/emacs-compat/compat
+;; Version: 1.0
+;; Keywords: lisp, maint
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; The Compat package on ELPA provides forward-compatibility
+;; definitions for other packages.  While mostly transparent, a
+;; minimal API is necessary whenever core definitions change calling
+;; conventions (e.g. `plist-get' can be invoked with a predicate from
+;; Emacs 29.1 onward).  For core packages on ELPA to be able to take
+;; advantage of this functionality, the macros `compat-function' and
+;; `compat-call' have to be available in the core, usable even if
+;; users do not have the Compat package installed, which this file
+;; ensures.
+
+;; Note that Compat is not a core package and this file is not
+;; available on GNU ELPA.
+
+;;; Code:
+
+(defmacro compat-function (fun)
+  "Return compatibility function symbol for FUN.
+This is a pseudo-compatibility stub for core packages on ELPA,
+that depend on the Compat package, whenever the user doesn't have
+the package installed on their current system."
+  `#',fun)
+
+(defmacro compat-call (fun &rest args)
+  "Call compatibility function or macro FUN with ARGS.
+This is a pseudo-compatibility stub for core packages on ELPA,
+that depend on the Compat package, whenever the user doesn't have
+the package installed on their current system."
+  (cons fun args))
+
+(provide 'compat)
+;;; compat.el ends here
diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 4b940b3f13b..bf0e27bc786 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -264,7 +264,7 @@
 (eval-when-compile (require 'subr-x))   ;For `string-empty-p' and `string-join'.
 (require 'treesit)
 (require 'pcase)
-(require 'compat nil 'noerror)
+(require 'compat)
 (require 'project nil 'noerror)
 (require 'seq)
 
-- 
2.39.2

Re: compat.el and Emacs unstable master branch features

[Daniel Mendler]

On 9/13/23 12:31, Philip Kaludercic wrote:
> Daniel Mendler <mail@daniel-mendler.de> writes:
> 
>> On 9/12/23 12:02, Philip Kaludercic wrote:
>>> Ihor Radchenko <yantar92@posteo.net> writes:
>>>
>>>> Daniel Mendler <mail@daniel-mendler.de> writes:
>>>>
>>>>> ... Providing a public API won't work
>>>>> since Compat is not included in Emacs itself. A design criterion of
>>>>> Compat is also to keep the public API surface as small as possible.
>>>>
>>>> Maybe it is the time to consider including the compat.el API into Emacs?
>>>> We are getting a number of :core packages published on ELPA and
>>>> naturally having to solve various compatibility problems.
>>>
>>> I am a bit behind wrt Compat development.  Are we talking about adding
>>> the `compat-call, etc. macros to the core?  So basically just a
>>>
>>>   (defmacro compat-call (&rest args) args)
>>>
>>> that would be overriden by compat's compat-call, as soon as that is
>>> loaded
>>
>> Yes, if the Emacs maintainers agree I think this could be done. Take
>> compat.el, remove the part which requires compat-29, and copy it to
>> lisp/ or lisp/emacs-lisp/. As you said, if Compat (the package) is
>> installed it will be preferred over the core compat.el. The advantage of
>> this move is that core package could require compat directly without
>> passing a noerror argument to require. Furthermore `compat-call' and
>> `compat-function' would be available and wouldn't have to be copied as
>> is currently the case for example in erc-compat.el.
> 
> That sounds good!  How does this look like:

Yes, this is what I meant. I forgot to mention one additional advantage
in my last mail: If the compat.el in core is registered with package.el
as builtin with version 30, the ELPA Compat package wouldn't get pulled
in by external packages which depend on Compat version 29. The core
compat.el version should then be bumped together with the Emacs version.

Daniel

Re: compat.el and Emacs unstable master branch features

[Ihor Radchenko]

Daniel Mendler <mail@daniel-mendler.de> writes:

>>> Yes, if the Emacs maintainers agree I think this could be done. Take
>>> compat.el, remove the part which requires compat-29, and copy it to
>>> lisp/ or lisp/emacs-lisp/. As you said, if Compat (the package) is
>>> installed it will be preferred over the core compat.el. The advantage of
>>> this move is that core package could require compat directly without
>>> passing a noerror argument to require. Furthermore `compat-call' and
>>> `compat-function' would be available and wouldn't have to be copied as
>>> is currently the case for example in erc-compat.el.
>> 
>> That sounds good!  How does this look like:
>
> Yes, this is what I meant. I forgot to mention one additional advantage
> in my last mail: If the compat.el in core is registered with package.el
> as builtin with version 30, the ELPA Compat package wouldn't get pulled
> in by external packages which depend on Compat version 29. The core
> compat.el version should then be bumped together with the Emacs version.

Nobody raised objections. I recommend sending the patch to debbugs (M-x
submit-emacs-patch), so that it can be seen by Emacs maintainers.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: compat.el and Emacs unstable master branch features

[Philip Kaludercic]

Ihor Radchenko <yantar92@posteo.net> writes:

> Daniel Mendler <mail@daniel-mendler.de> writes:
>
>>>> Yes, if the Emacs maintainers agree I think this could be done. Take
>>>> compat.el, remove the part which requires compat-29, and copy it to
>>>> lisp/ or lisp/emacs-lisp/. As you said, if Compat (the package) is
>>>> installed it will be preferred over the core compat.el. The advantage of
>>>> this move is that core package could require compat directly without
>>>> passing a noerror argument to require. Furthermore `compat-call' and
>>>> `compat-function' would be available and wouldn't have to be copied as
>>>> is currently the case for example in erc-compat.el.
>>> 
>>> That sounds good!  How does this look like:
>>
>> Yes, this is what I meant. I forgot to mention one additional advantage
>> in my last mail: If the compat.el in core is registered with package.el
>> as builtin with version 30, the ELPA Compat package wouldn't get pulled
>> in by external packages which depend on Compat version 29. The core
>> compat.el version should then be bumped together with the Emacs version.
>
> Nobody raised objections. I recommend sending the patch to debbugs (M-x
> submit-emacs-patch), so that it can be seen by Emacs maintainers.

See bug#66554.

-- 
Philip Kaludercic