Re: master 3bbe9e6: Avoid writing empty abbrev tables

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Glenn Morris]

> branch: master
> commit 3bbe9e609138ae88a4c98bcee0da8fcf8b4a3e80
> Author: Allen Li <darkfeline@felesatra.moe>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     Avoid writing empty abbrev tables

Hi, this causes some abbrev tests to fail.
Ref eg https://hydra.nixos.org/build/82163465

In general, after changing lisp/foo.el, running the tests from
test/lisp/foo.el only takes a few seconds.

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Allen Li]

Oops, I'll take a look
On Sat, Sep 29, 2018 at 5:52 PM Glenn Morris <rgm@gnu.org> wrote:
>
>
> > branch: master
> > commit 3bbe9e609138ae88a4c98bcee0da8fcf8b4a3e80
> > Author: Allen Li <darkfeline@felesatra.moe>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> >
> >     Avoid writing empty abbrev tables
>
> Hi, this causes some abbrev tests to fail.
> Ref eg https://hydra.nixos.org/build/82163465
>
> In general, after changing lisp/foo.el, running the tests from
> test/lisp/foo.el only takes a few seconds.

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Allen Li]

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

Attached patch reworking the feature which fixes the tests.
Explanation in patch.
On Sat, Sep 29, 2018 at 9:06 PM Allen Li <darkfeline@felesatra.moe> wrote:
>
> Oops, I'll take a look
> On Sat, Sep 29, 2018 at 5:52 PM Glenn Morris <rgm@gnu.org> wrote:
> >
> >
> > > branch: master
> > > commit 3bbe9e609138ae88a4c98bcee0da8fcf8b4a3e80
> > > Author: Allen Li <darkfeline@felesatra.moe>
> > > Commit: Eli Zaretskii <eliz@gnu.org>
> > >
> > >     Avoid writing empty abbrev tables
> >
> > Hi, this causes some abbrev tests to fail.
> > Ref eg https://hydra.nixos.org/build/82163465
> >
> > In general, after changing lisp/foo.el, running the tests from
> > test/lisp/foo.el only takes a few seconds.

[-- Attachment #2: 0001-Rework-empty-abbrev-table-omitting.patch --]
[-- Type: text/x-patch, Size: 6334 bytes --]

From 8f3aadf85ef9d515f6140dad89be426128485981 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sat, 29 Sep 2018 15:19:04 -0700
Subject: [PATCH] Rework empty abbrev table omitting

There were two problems with the original implementation:

1. It changed the behavior of insert-abbrev-table-description when
READABLE is nil to sometimes insert one Emacs Lisp expression and
sometimes insert nothing.
2. It broke the tests.

This commit reworks this so that insert-abbrev-table-description
always inserts an expressions even if no abbrevs need to be saved and
making only write-abbrev-file check that a table has any abbrevs to
save before calling insert-abbrev-table-description.  This duplicates
the work of filtering the table for savable abbrevs, but the benefit
of keeping the API is worth it.

* doc/lispref/abbrevs.texi (Abbrev Tables): Update documentation.
* lisp/abbrev.el (write-abbrev-file): Skip tables without user abbrevs
(insert-abbrev-table-description): Always insert the define
expression.
(abbrev--table-symbols): New function.
* test/lisp/abbrev-tests.el (abbrev--table-symbols-test):
Add test for abbrev--table-symbols.
---
 doc/lispref/abbrevs.texi  |  7 +++---
 lisp/abbrev.el            | 52 ++++++++++++++++++++++-----------------
 test/lisp/abbrev-tests.el |  8 ++++++
 3 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/doc/lispref/abbrevs.texi b/doc/lispref/abbrevs.texi
index 4c9e653cb1..1e9471ba27 100644
--- a/doc/lispref/abbrevs.texi
+++ b/doc/lispref/abbrevs.texi
@@ -122,9 +122,7 @@ Abbrev Tables
 description is a Lisp expression---a call to @code{define-abbrev-table}
 that would define @var{name} as it is currently defined, but without
 the system abbrevs.  (The mode or package using @var{name} is supposed
-to add these to @var{name} separately.)  If the Lisp expression would
-not define any abbrevs (i.e.@: it defines an empty abbrev table), this
-function inserts nothing.
+to add these to @var{name} separately.)
 @end defun
 
 @node Defining Abbrevs
@@ -234,7 +232,8 @@ Abbrev Files
 Save all abbrev definitions (except system abbrevs), for all abbrev
 tables listed in @code{abbrev-table-name-list}, in the file
 @var{filename}, in the form of a Lisp program that when loaded will
-define the same abbrevs.  If @var{filename} is @code{nil} or omitted,
+define the same abbrevs.  Tables that do not have any abbrevs to save
+are omitted.  If @var{filename} is @code{nil} or omitted,
 @code{abbrev-file-name} is used.  This function returns @code{nil}.
 @end deffn
 
diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index e1fd366ba9..20a967d7d6 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -251,7 +251,8 @@ write-abbrev-file
 		     (lambda (s1 s2)
 		       (string< (symbol-name s1)
 				(symbol-name s2)))))
-	(insert-abbrev-table-description table nil))
+	(if (abbrev--table-symbols table)
+            (insert-abbrev-table-description table nil)))
       (when (unencodable-char-position (point-min) (point-max) 'utf-8)
 	(setq coding-system-for-write
 	      (if (> emacs-major-version 24)
@@ -937,33 +938,38 @@ insert-abbrev-table-description
 If READABLE is nil, an expression is inserted.  The expression is
 a call to `define-abbrev-table' that when evaluated will define
 the abbrev table NAME exactly as it is currently defined.
-Abbrevs marked as \"system abbrevs\" are ignored.  If the
-resulting expression would not define any abbrevs, nothing is
-inserted."
+Abbrevs marked as \"system abbrevs\" are ignored."
+  (let ((table (symbol-value name))
+        (symbols (abbrev--table-symbols name readable)))
+    (setq symbols (sort symbols 'string-lessp))
+    (let ((standard-output (current-buffer)))
+      (if readable
+          (progn
+            (insert "(")
+            (prin1 name)
+            (insert ")\n\n")
+            (mapc 'abbrev--describe symbols)
+            (insert "\n\n"))
+        (insert "(define-abbrev-table '")
+        (prin1 name)
+        (if (null symbols)
+            (insert " '())\n\n")
+          (insert "\n  '(\n")
+          (mapc 'abbrev--write symbols)
+          (insert "   ))\n\n")))
+      nil)))
+
+(defun abbrev--table-symbols (name &optional system)
+  "Return the user abbrev symbols in the abbrev table named NAME.
+NAME is a symbol whose value is an abbrev table.  System abbrevs
+are omitted unless SYSTEM is non-nil."
   (let ((table (symbol-value name))
         (symbols ()))
     (mapatoms (lambda (sym)
-                (if (and (symbol-value sym) (or readable (not (abbrev-get sym :system))))
+                (if (and (symbol-value sym) (or system (not (abbrev-get sym :system))))
                     (push sym symbols)))
               table)
-    (when symbols
-      (setq symbols (sort symbols 'string-lessp))
-      (let ((standard-output (current-buffer)))
-        (if readable
-            (progn
-              (insert "(")
-              (prin1 name)
-              (insert ")\n\n")
-              (mapc 'abbrev--describe symbols)
-              (insert "\n\n"))
-          (insert "(define-abbrev-table '")
-          (prin1 name)
-          (if (null symbols)
-              (insert " '())\n\n")
-            (insert "\n  '(\n")
-            (mapc 'abbrev--write symbols)
-            (insert "   ))\n\n")))
-        nil))))
+    symbols))
 
 (defun define-abbrev-table (tablename definitions
                                       &optional docstring &rest props)
diff --git a/test/lisp/abbrev-tests.el b/test/lisp/abbrev-tests.el
index facf097815..e50f931cef 100644
--- a/test/lisp/abbrev-tests.el
+++ b/test/lisp/abbrev-tests.el
@@ -64,6 +64,14 @@ setup-test-abbrev-table-with-props
     (should (= (length table) obarray-default-size))
     (should (eq (abbrev-table-get table 'foo) 'bar))))
 
+(ert-deftest abbrev--table-symbols-test ()
+  (let ((ert-test-abbrevs (setup-test-abbrev-table)))
+    (define-abbrev ert-test-abbrevs "sys" "system abbrev" nil :system t)
+    (should (equal (mapcar #'symbol-name (abbrev--table-symbols 'ert-test-abbrevs))
+                   '("a-e-t")))
+    (should (equal (mapcar #'symbol-name (abbrev--table-symbols 'ert-test-abbrevs t))
+                   '("a-e-t" "sys")))))
+
 (ert-deftest abbrev-table-get-put-test ()
   (let ((table (make-abbrev-table)))
     (should-not (abbrev-table-get table 'foo))
-- 
2.19.0

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Eli Zaretskii]

> From: Allen Li <darkfeline@felesatra.moe>
> Date: Sat, 29 Sep 2018 22:28:52 +0000
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> Attached patch reworking the feature which fixes the tests.
> Explanation in patch.

Thanks, but is this relative to the current master branch?  If not,
could you please rebase on master's HEAD and resubmit?

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Allen Li]

On Sun, Sep 30, 2018 at 5:36 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Allen Li <darkfeline@felesatra.moe>
> > Date: Sat, 29 Sep 2018 22:28:52 +0000
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > Attached patch reworking the feature which fixes the tests.
> > Explanation in patch.
>
> Thanks, but is this relative to the current master branch?  If not,
> could you please rebase on master's HEAD and resubmit?

Yes, it's on top of 6217746dd64b43a2a2b3b66ab50cfbbfc984f36c

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Eli Zaretskii]

> From: Allen Li <darkfeline@felesatra.moe>
> Date: Sun, 30 Sep 2018 07:25:15 +0000
> Cc: rgm@gnu.org, Emacs developers <emacs-devel@gnu.org>
> 
> On Sun, Sep 30, 2018 at 5:36 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Allen Li <darkfeline@felesatra.moe>
> > > Date: Sat, 29 Sep 2018 22:28:52 +0000
> > > Cc: Emacs developers <emacs-devel@gnu.org>
> > >
> > > Attached patch reworking the feature which fixes the tests.
> > > Explanation in patch.
> >
> > Thanks, but is this relative to the current master branch?  If not,
> > could you please rebase on master's HEAD and resubmit?
> 
> Yes, it's on top of 6217746dd64b43a2a2b3b66ab50cfbbfc984f36c

OK, thanks.  But then the NEWS entry needs to be updated, doesn't it?

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Allen Li]

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

On Sun, Sep 30, 2018 at 7:44 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> OK, thanks.  But then the NEWS entry needs to be updated, doesn't it?

Done

[-- Attachment #2: 0001-Rework-empty-abbrev-table-omitting.patch --]
[-- Type: text/x-patch, Size: 7036 bytes --]

From 233db0c8a61416e2e95ba2c39d80ef25388cab60 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sat, 29 Sep 2018 15:19:04 -0700
Subject: [PATCH] Rework empty abbrev table omitting

There were two problems with the original implementation:

1. It changed the behavior of insert-abbrev-table-description when
READABLE is nil to sometimes insert one Emacs Lisp expression and
sometimes insert nothing.
2. It broke the tests.

This commit reworks this so that insert-abbrev-table-description
always inserts an expressions even if no abbrevs need to be saved and
making only write-abbrev-file check that a table has any abbrevs to
save before calling insert-abbrev-table-description.  This duplicates
the work of filtering the table for savable abbrevs, but the benefit
of keeping the API is worth it.

* doc/lispref/abbrevs.texi (Abbrev Tables): Update documentation.
* lisp/abbrev.el (write-abbrev-file): Skip tables without user abbrevs
(insert-abbrev-table-description): Always insert the define
expression.
(abbrev--table-symbols): New function.
* test/lisp/abbrev-tests.el (abbrev--table-symbols-test):
Add test for abbrev--table-symbols.
---
 doc/lispref/abbrevs.texi  |  7 +++---
 etc/NEWS                  |  7 +++---
 lisp/abbrev.el            | 52 ++++++++++++++++++++++-----------------
 test/lisp/abbrev-tests.el |  8 ++++++
 4 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/doc/lispref/abbrevs.texi b/doc/lispref/abbrevs.texi
index 4c9e653cb1..1e9471ba27 100644
--- a/doc/lispref/abbrevs.texi
+++ b/doc/lispref/abbrevs.texi
@@ -122,9 +122,7 @@ Abbrev Tables
 description is a Lisp expression---a call to @code{define-abbrev-table}
 that would define @var{name} as it is currently defined, but without
 the system abbrevs.  (The mode or package using @var{name} is supposed
-to add these to @var{name} separately.)  If the Lisp expression would
-not define any abbrevs (i.e.@: it defines an empty abbrev table), this
-function inserts nothing.
+to add these to @var{name} separately.)
 @end defun
 
 @node Defining Abbrevs
@@ -234,7 +232,8 @@ Abbrev Files
 Save all abbrev definitions (except system abbrevs), for all abbrev
 tables listed in @code{abbrev-table-name-list}, in the file
 @var{filename}, in the form of a Lisp program that when loaded will
-define the same abbrevs.  If @var{filename} is @code{nil} or omitted,
+define the same abbrevs.  Tables that do not have any abbrevs to save
+are omitted.  If @var{filename} is @code{nil} or omitted,
 @code{abbrev-file-name} is used.  This function returns @code{nil}.
 @end deffn
 
diff --git a/etc/NEWS b/etc/NEWS
index 7e7de165ec..ca7af2daef 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -252,10 +252,9 @@ case does not match.
 for abbrevs that have them.
 
 +++
-** 'insert-abbrev-table-description' skips empty tables.
-'insert-abbrev-table-description' skips inserting empty tables when
-inserting non-readable tables.  By extension, this makes
-'write-abbrev-file' skip writing empty tables.
+** 'write-abbrev-file' skips empty tables.
+'write-abbrev-file' now skips inserting a 'define-abbrev-table' form for
+tables which do not have any non-system abbrevs to save.
 
 +++
 ** The new functions and commands 'text-property-search-forward' and
diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index e1fd366ba9..20a967d7d6 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -251,7 +251,8 @@ write-abbrev-file
 		     (lambda (s1 s2)
 		       (string< (symbol-name s1)
 				(symbol-name s2)))))
-	(insert-abbrev-table-description table nil))
+	(if (abbrev--table-symbols table)
+            (insert-abbrev-table-description table nil)))
       (when (unencodable-char-position (point-min) (point-max) 'utf-8)
 	(setq coding-system-for-write
 	      (if (> emacs-major-version 24)
@@ -937,33 +938,38 @@ insert-abbrev-table-description
 If READABLE is nil, an expression is inserted.  The expression is
 a call to `define-abbrev-table' that when evaluated will define
 the abbrev table NAME exactly as it is currently defined.
-Abbrevs marked as \"system abbrevs\" are ignored.  If the
-resulting expression would not define any abbrevs, nothing is
-inserted."
+Abbrevs marked as \"system abbrevs\" are ignored."
+  (let ((table (symbol-value name))
+        (symbols (abbrev--table-symbols name readable)))
+    (setq symbols (sort symbols 'string-lessp))
+    (let ((standard-output (current-buffer)))
+      (if readable
+          (progn
+            (insert "(")
+            (prin1 name)
+            (insert ")\n\n")
+            (mapc 'abbrev--describe symbols)
+            (insert "\n\n"))
+        (insert "(define-abbrev-table '")
+        (prin1 name)
+        (if (null symbols)
+            (insert " '())\n\n")
+          (insert "\n  '(\n")
+          (mapc 'abbrev--write symbols)
+          (insert "   ))\n\n")))
+      nil)))
+
+(defun abbrev--table-symbols (name &optional system)
+  "Return the user abbrev symbols in the abbrev table named NAME.
+NAME is a symbol whose value is an abbrev table.  System abbrevs
+are omitted unless SYSTEM is non-nil."
   (let ((table (symbol-value name))
         (symbols ()))
     (mapatoms (lambda (sym)
-                (if (and (symbol-value sym) (or readable (not (abbrev-get sym :system))))
+                (if (and (symbol-value sym) (or system (not (abbrev-get sym :system))))
                     (push sym symbols)))
               table)
-    (when symbols
-      (setq symbols (sort symbols 'string-lessp))
-      (let ((standard-output (current-buffer)))
-        (if readable
-            (progn
-              (insert "(")
-              (prin1 name)
-              (insert ")\n\n")
-              (mapc 'abbrev--describe symbols)
-              (insert "\n\n"))
-          (insert "(define-abbrev-table '")
-          (prin1 name)
-          (if (null symbols)
-              (insert " '())\n\n")
-            (insert "\n  '(\n")
-            (mapc 'abbrev--write symbols)
-            (insert "   ))\n\n")))
-        nil))))
+    symbols))
 
 (defun define-abbrev-table (tablename definitions
                                       &optional docstring &rest props)
diff --git a/test/lisp/abbrev-tests.el b/test/lisp/abbrev-tests.el
index facf097815..e50f931cef 100644
--- a/test/lisp/abbrev-tests.el
+++ b/test/lisp/abbrev-tests.el
@@ -64,6 +64,14 @@ setup-test-abbrev-table-with-props
     (should (= (length table) obarray-default-size))
     (should (eq (abbrev-table-get table 'foo) 'bar))))
 
+(ert-deftest abbrev--table-symbols-test ()
+  (let ((ert-test-abbrevs (setup-test-abbrev-table)))
+    (define-abbrev ert-test-abbrevs "sys" "system abbrev" nil :system t)
+    (should (equal (mapcar #'symbol-name (abbrev--table-symbols 'ert-test-abbrevs))
+                   '("a-e-t")))
+    (should (equal (mapcar #'symbol-name (abbrev--table-symbols 'ert-test-abbrevs t))
+                   '("a-e-t" "sys")))))
+
 (ert-deftest abbrev-table-get-put-test ()
   (let ((table (make-abbrev-table)))
     (should-not (abbrev-table-get table 'foo))
-- 
2.19.0

Re: master 3bbe9e6: Avoid writing empty abbrev tables

[Eli Zaretskii]

> From: Allen Li <darkfeline@felesatra.moe>
> Date: Tue, 2 Oct 2018 05:49:50 +0000
> Cc: rgm@gnu.org, Emacs developers <emacs-devel@gnu.org>
> 
> On Sun, Sep 30, 2018 at 7:44 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > OK, thanks.  But then the NEWS entry needs to be updated, doesn't it?
> 
> Done

Thanks, pushed.