GNU bug report logs - #67153
[PATCH 2/2] Add 5 docstrings to abbrev.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 67153 in the body.
You can then email your comments to 67153 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: bug-gnu-emacs <at> gnu.org, Richard Stallman <rms <at> gnu.org>, Gerd
 Möllmann
 <gerd <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Mon, 13 Nov 2023 22:38:39 +0000

[Message part 1 (text/plain, inline)]

Tags: patch

This patch adds 5 docstrings to abbrev.el where there were none before.
This now completes all docstrings in this Lisp file.

The patch is a documentation patch based on emacs-29.
Please let me know if this is ready to install or if changes are needed.

[0002-Add-5-docstrings-to-abbrev.el.patch (text/patch, attachment)]

Message #8 received at 67153 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: 67153 <at> debbugs.gnu.org, gerd <at> gnu.org, rms <at> gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#67153: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Tue, 14 Nov 2023 16:17:54 +0200

> Date: Mon, 13 Nov 2023 22:38:39 +0000
> From:  Jeremy Bryant via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> This patch adds 5 docstrings to abbrev.el where there were none before.
> This now completes all docstrings in this Lisp file.
> 
> The patch is a documentation patch based on emacs-29.
> Please let me know if this is ready to install or if changes are needed.

Thanks, see a few comments below.

>  (defun prepare-abbrev-list-buffer (&optional local)
> +  "Return *Abbrevs* buffer for the caller to select or display.
> +
> +LOCAL is a flag, if non-nil display only local abbrevs.
> +"

That newline before the closing quote is redundant,

>  (defun add-abbrev (table type arg)
> +  "Define an abbrev in abbrev TABLE whose expansion is last word before point.

The first sentence of a doc string should preferably mention the
mandatory arguments (TYPE and ARG).  If the result is too long to fit
on a single line, consider saying only the main part there, and then
describing the details in the following lines.

> +TYPE of abbrev includes \"Global\", \"Mode\".

"Includes"? are there other types?

>  (defun inverse-add-abbrev (table type arg)
> +  "Define the word before point as as an abbrev in TABLE.
                                   ^^^^^
Typo.

> +ARG means use the ARG-th word before point as the
> +abbreviation.  Negative ARG means use the ARG-th word after point.
> +
> +This command reads the expansion from the minibuffer, defines the
> +abbrev, and then expands the abbreviation in the current buffer.
> +
> +TYPE of abbrev can be \"Global\", \"Mode\".
> +
> +See `inverse-add-global-n', `inverse-add-mode-abbrev' for caller examples.
> +See also `add-abbrev', which performs the opposite task."

Same comments here.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: Gerd Möllmann <gerd <at> gnu.org>, bug-gnu-emacs <at> gnu.org,
 Richard Stallman <rms <at> gnu.org>
Subject: Re: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Tue, 14 Nov 2023 14:23:01 -0500

> This patch adds 5 docstrings to abbrev.el where there were none before.

Thanks.  See some comments below.


        Stefan


>  (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.
- 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.

> +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".

> +TYPE of abbrev includes \"Global\", \"Mode\".

Here you made the same mistake of documenting what the callers do rather
than what the function does.

> +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.

>  (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? ...).

>  (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.


        Stefan

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Gerd Möllmann <gerd <at> gnu.org>, bug-gnu-emacs <at> gnu.org,
 Richard Stallman <rms <at> gnu.org>
Subject: Re: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Wed, 15 Nov 2023 23:24:48 +0000

[0001-Add-5-docstrings-to-abbrev.el.patch (text/x-diff, attachment)]

[Message part 2 (text/plain, inline)]

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

Further clarifications below:-


Stefan Monnier <monnier <at> iro.umontreal.ca> 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 <at> 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

Message #17 received at 67153 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: 67153 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#67153: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Thu, 16 Nov 2023 08:47:18 +0200

> Cc: gerd <at> gnu.org, 67153 <at> debbugs.gnu.org, rms <at> gnu.org
> Date: Wed, 15 Nov 2023 23:24:48 +0000
> From:  Jeremy Bryant via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
>  (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."

Our style for expressing what you say in the last sentence is

  If LOCAL is non-nil, include in the buffer only the local abbrevs.

("Display" is not appropriate here, since this function doesn't
display the buffer.)

>  (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.
        ^^^^^^^
This is not a command.

> +ARG of zero means the entire region is the expansion.
> +If there's an active region, use that as the expansion.

Is the second sentence conditioned on the first, i.e., does the
function use the active region only when ARG is zero?  If so, the
second sentence should be removed.  If use of the active region is NOT
conditioned on ARG, I would reword the above:

  If there's an active region, use the region as the expansion.
  ARG of zero means the region is the expansion, even if it is
  inactive.

>  (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."

Same comments here.

>  (defun abbrev--possibly-save (query &optional arg)
> +  "Maybe save abbrevs: Hook function for use by `save-some-buffer-functions'.

I'd say here just

  Hook function for use by `save-some-buffer-functions'.

and add the "Maybe save" bit as a separate second sentence.

> +Associated meaning for QUERY and ARG."

I couldn't parse this sentence.

Thanks.

Message #20 received at 67153 <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 67153 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#67153: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Thu, 16 Nov 2023 23:48:04 +0000

[0001-Add-5-docstrings-to-abbrev.el.patch (text/x-diff, attachment)]

[Message part 2 (text/plain, inline)]

Thank you for your patience in explaining what is required.
New patch attached, with comments below.


Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: gerd <at> gnu.org, 67153 <at> debbugs.gnu.org, rms <at> gnu.org
>> Date: Wed, 15 Nov 2023 23:24:48 +0000
>> From:  Jeremy Bryant via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>>  (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."
>
> Our style for expressing what you say in the last sentence is
>
>   If LOCAL is non-nil, include in the buffer only the local abbrevs.
>
> ("Display" is not appropriate here, since this function doesn't
> display the buffer.)

Noted, fixed

>
>>  (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.
>         ^^^^^^^
> This is not a command.

Reworded
>
>> +ARG of zero means the entire region is the expansion.
>> +If there's an active region, use that as the expansion.
>
> Is the second sentence conditioned on the first, i.e., does the
> function use the active region only when ARG is zero?  If so, the
> second sentence should be removed.  If use of the active region is NOT
> conditioned on ARG, I would reword the above:
>
>   If there's an active region, use the region as the expansion.
>   ARG of zero means the region is the expansion, even if it is
>   inactive.
>

Fixed by removing sentence
>>  (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."
>
> Same comments here.

Reworded; region comment N/A.
>
>>  (defun abbrev--possibly-save (query &optional arg)
>> +  "Maybe save abbrevs: Hook function for use by `save-some-buffer-functions'.
>
> I'd say here just
>
>   Hook function for use by `save-some-buffer-functions'.
>
> and add the "Maybe save" bit as a separate second sentence.

Reworded by moving comment.

>
>> +Associated meaning for QUERY and ARG."
>
> I couldn't parse this sentence.
>
> Thanks.

Initially I read Stefan's suggestion

"
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.
"

and left this as guide to arguments, but have now removed arguments
entirely.

Please let me know if this is good to install or any other changes needed.

Message #25 received at 67153-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: 67153-done <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#67153: [PATCH 2/2] Add 5 docstrings to abbrev.el
Date: Fri, 17 Nov 2023 10:17:12 +0200

> From: Jeremy Bryant <jb <at> jeremybryant.net>
> Cc: monnier <at> iro.umontreal.ca, 67153 <at> debbugs.gnu.org
> Date: Thu, 16 Nov 2023 23:48:04 +0000
> 
> Thank you for your patience in explaining what is required.
> New patch attached, with comments below.

Thanks, installed on the emacs-29 branch, and closing the bug.

Please note my minor changes in the log message, and try to follow
these conventions in the future.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.