Subject: bug#67153: [PATCH 2/2] Add 5 docstrings to abbrev.el

From: Jeremy Bryant <[email-masked]>
Date: Wed, 15 Nov 2023 23:24:48 +0000

>From 2e7f167ae4c6cac02d530a1c9d529c3240eb4187 Mon Sep 17 00:00:00 2001
From: Jeremy Bryant <[email-masked]>
Date: Wed, 15 Nov 2023 23:15:46 +0000
Subject: [PATCH] Add 5 docstrings to abbrev.el

* lisp/abbrev.el (prepare-abbrev-list-buffer)
(add-abbrev)
(inverse-add-abbrev)
(abbrev--describe)
(abbrev--possibly-save): Add docstrings.
---
 lisp/abbrev.el | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index e1311dbc83b..6f62e088d64 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -122,6 +122,9 @@ abbrev-table-name
     found))
 
 (defun prepare-abbrev-list-buffer (&optional local)
+  "Return buffer listing abbreviations and expansions for each abbrev table.
+
+LOCAL is a flag, if non-nil display only local abbrevs."
   (let ((local-table local-abbrev-table))
     (with-current-buffer (get-buffer-create "*Abbrevs*")
       (erase-buffer)
@@ -333,6 +336,21 @@ add-global-abbrev
   (add-abbrev global-abbrev-table "Global" arg))
 
 (defun add-abbrev (table type arg)
+  "Define abbrev in TABLE, whose expansion is ARG words before point.
+This command reads the abbreviation from the minibuffer, with prompt TYPE.
+
+ARG of zero means the entire region is the expansion.
+If there's an active region, use that as the expansion.
+
+A negative ARG means to undefine the specified abbrev.
+
+TYPE is an arbitrary string used to prompt user for the kind of
+abbrev, such as \"Global\", \"Mode\".  (This has no influence on the
+choice of the actual TABLE).
+
+See `inverse-add-abbrev' for the opposite task.
+
+Don't use this function in a Lisp program; use `define-abbrev' instead."
   (let ((exp
          (cond
           ((or (and (null arg) (use-region-p))
@@ -353,7 +371,7 @@ add-abbrev
     (if (or (null exp)
 	    (not (abbrev-expansion name table))
 	    (y-or-n-p (format "%s expands into \"%s\"; redefine? "
-			      name (abbrev-expansion name table))))
+                              name (abbrev-expansion name table))))
 	(define-abbrev table (downcase name) exp))))
 
 (defun inverse-add-mode-abbrev (n)
@@ -393,6 +411,18 @@ inverse-add-global-abbrev
   (inverse-add-abbrev global-abbrev-table "Global" n))
 
 (defun inverse-add-abbrev (table type arg)
+  "Define the word before point as an abbrev in TABLE.
+This command reads the expansion from the minibuffer, using prompt TYPE,
+defines the abbrev, and then expands the abbreviation in the current buffer.
+
+ARG means use the ARG-th word before point as the abbreviation.
+Negative ARG means use the ARG-th word after point.
+
+TYPE is an arbitrary string used to prompt user for the kind of
+abbrev, such as \"Global\", \"Mode\".  (This has no influence on the
+choice of the actual TABLE).
+
+See also `add-abbrev', which performs the opposite task."
   (let (name exp start end)
     (save-excursion
       (forward-word (1+ (- arg)))
@@ -1102,6 +1132,8 @@ abbrev--write
   (insert ")\n"))
 
 (defun abbrev--describe (sym)
+  "Describe abbrev SYM.
+Print on `standard-output' the abbrev, count of use, expansion."
   (when (symbol-value sym)
     (prin1 (symbol-name sym))
     (if (null (abbrev-get sym :system))
@@ -1243,6 +1275,8 @@ edit-abbrevs-mode
   (setq font-lock-multiline nil))
 
 (defun abbrev--possibly-save (query &optional arg)
+  "Maybe save abbrevs: Hook function for use by `save-some-buffer-functions'.
+Associated meaning for QUERY and ARG."
   ;; Query mode.
   (if (eq query 'query)
       (and save-abbrevs abbrevs-changed)
-- 
2.40.1

Attached is a revised patch addressing comments of Eli and Stefan.

Further clarifications below:-

Stefan Monnier <[email-masked]> writes:

>>  (defun prepare-abbrev-list-buffer (&optional local)
>> +  "Return *Abbrevs* buffer for the caller to select or display.
>
> A few points:
>
> - I don't think the docstring should document to the name of the buffer.
> - Docstrings should usually say what the function does rather than what
>   the callers are expected to do with the result.

Initially, I had simply lifted the text from the file ChangeLog.1
1985-12-13  Richard M. Stallman  (rms@prep)
        * abbrev.el (prepare-abbrev-list-buffer, list-abbrevs,
	edit-abbrevs):
        Some cleanups.  prepare-... now does all the work and
	returns the buffer for the caller to select or display.

But I have now rewritten.

> - To me, the above description makes it sound like the function does
>   little more than `(get-buffer "*Abbrevs*")`.
>   IOW, it should say that it fills the buffer with "some" representation
>   of "some" abbrevs.
>
>> +LOCAL is a flag, if non-nil display only local abbrevs.
>> +"
>
> We don't like our docstrings to end with a newline.

Fixed.

>
>> +A negative ARG means to undefine the specified abbrev.
> [...]
>> +This command reads the abbreviation from the minibuffer.
>
> We should say this before referring to it via "the specified abbrev".

Fixed in both add-abbrev and inverse-add-abbrev.
I initially adapted the docstring from surrounding callers like
add-global-abbrev, but have now rewritten.

>
>> +TYPE of abbrev includes \"Global\", \"Mode\".
>
> Here you made the same mistake of documenting what the callers do rather
> than what the function does.

Fixed, rewritten, it turns out the ARG string is purely descriptive.

>
>> +See `add-global-abbrev', `add-mode-abbrev' for caller examples.
>
> We usually don't include this in docstrings.
> We have `xref` and `grep` for those kinds of things.

Fixed

>>  (defun abbrev--describe (sym)
>> +  "Describe abbrev SYM.
>> +Used to generate a table by `insert-abbrev-table-description'."
>
> Similarly here I wouldn't mention the caller.  Instead I'd try and
> explain what kind of description is generated and where it's placed
> (at point in the current buffer?  As a return values? ...).

Fixed with this detail after examining the code.
>
>>  (defun abbrev--possibly-save (query &optional arg)
>> +  "Hook function for use by `save-some-buffer-functions'.
>> +Associated meaning for QUERY and ARG."
>
> Hook functions are one of the exceptions where it's often OK to refer to
> how it's used in the docstring, like you do here (e.g. so we don't have
> to repeat what `query` and `arg` are for).
> But it should also say what it actually does.

Fixed.

>         Stefan