bug#72915: Docstrings of add-hook and remove-hook improvement?

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Tomas Nordin]

Hello List

In bug#70820 August 14, Stefan mentions that it is a common confusion to
think of the functions in a hook as hooks. It got my attention because I
belong to the confused ones every second year or so adding or removing
functions from a hook.

I suggest the provided patch as a small improvement of the function
documentation of add-hook and remove-hook. Maybe it doesn't mitigate the
confusion mentioned that much, but it seem to align better with the
manual as I read it.

What do you think?

In add-hook doc, lift up the paragraph about HOOK and FUNCTION and
remove the mention about first setting the HOOK to nil. I think that is
something internal to the add-hook function and not relevant to the
programmer calling the add-hook function? And then say that the
resulting hook will be a list both when the HOOK symbol is void or a
single function.

In remove-hook, stick to the notion that a hook contain functions to
run, not hooks.

This notion though is a bit confusing in relation to the names of those
functions, but that's another story I guess.

The following on top of emacs-30.

diff --git a/lisp/subr.el b/lisp/subr.el
index 28ba30f584e..e60c4119c60 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2090,6 +2090,10 @@ add-hook
   "Add to the value of HOOK the function FUNCTION.
 FUNCTION is not added if already present.
 
+HOOK should be a symbol.  If HOOK is void, or if HOOK's value is a
+single function, it is changed to a list of functions (containing only
+FUNCTION in the void case).
+
 The place where the function is added depends on the DEPTH
 parameter.  DEPTH defaults to 0.  By convention, it should be
 a number between -100 and 100 where 100 means that the function
@@ -2108,10 +2112,6 @@ add-hook
 buffer-local value.  That acts as a flag to run the hook
 functions of the global value as well as in the local value.
 
-HOOK should be a symbol.  If HOOK is void, it is first set to
-nil.  If HOOK's value is a single function, it is changed to a
-list of functions.
-
 FUNCTION may be any valid function, but it's recommended to use a
 function symbol and not a lambda form.  Using a symbol will
 ensure that the function is not re-added if the function is
@@ -2179,7 +2179,7 @@ remove-hook
   "Remove from the value of HOOK the function FUNCTION.
 HOOK should be a symbol, and FUNCTION may be any valid function.  If
 FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
-list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
+list of functions to run in HOOK, then nothing is done.  See `add-hook'.
 
 The optional third argument, LOCAL, if non-nil, says to modify
 the hook's buffer-local value rather than its default value.

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Stefan Kangas]

Tomas Nordin <tomasn@posteo.net> writes:

> diff --git a/lisp/subr.el b/lisp/subr.el
> index 28ba30f584e..e60c4119c60 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -2090,6 +2090,10 @@ add-hook
>    "Add to the value of HOOK the function FUNCTION.
>  FUNCTION is not added if already present.
>
> +HOOK should be a symbol.  If HOOK is void, or if HOOK's value is a
> +single function, it is changed to a list of functions (containing only
> +FUNCTION in the void case).
> +
>  The place where the function is added depends on the DEPTH
>  parameter.  DEPTH defaults to 0.  By convention, it should be
>  a number between -100 and 100 where 100 means that the function
> @@ -2108,10 +2112,6 @@ add-hook
>  buffer-local value.  That acts as a flag to run the hook
>  functions of the global value as well as in the local value.
>
> -HOOK should be a symbol.  If HOOK is void, it is first set to
> -nil.  If HOOK's value is a single function, it is changed to a
> -list of functions.
> -
>  FUNCTION may be any valid function, but it's recommended to use a
>  function symbol and not a lambda form.  Using a symbol will
>  ensure that the function is not re-added if the function is
> @@ -2179,7 +2179,7 @@ remove-hook
>    "Remove from the value of HOOK the function FUNCTION.
>  HOOK should be a symbol, and FUNCTION may be any valid function.  If
>  FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
> -list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
> +list of functions to run in HOOK, then nothing is done.  See `add-hook'.
>
>  The optional third argument, LOCAL, if non-nil, says to modify
>  the hook's buffer-local value rather than its default value.

LGTM.

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Eli Zaretskii]

> From: Tomas Nordin <tomasn@posteo.net>
> Date: Sat, 31 Aug 2024 12:36:22 +0000
> 
> The following on top of emacs-30.
> 
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 28ba30f584e..e60c4119c60 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -2090,6 +2090,10 @@ add-hook
>    "Add to the value of HOOK the function FUNCTION.
>  FUNCTION is not added if already present.
>  
> +HOOK should be a symbol.  If HOOK is void, or if HOOK's value is a
> +single function, it is changed to a list of functions (containing only
> +FUNCTION in the void case).
> +
>  The place where the function is added depends on the DEPTH
>  parameter.  DEPTH defaults to 0.  By convention, it should be
>  a number between -100 and 100 where 100 means that the function
> @@ -2108,10 +2112,6 @@ add-hook
>  buffer-local value.  That acts as a flag to run the hook
>  functions of the global value as well as in the local value.
>  
> -HOOK should be a symbol.  If HOOK is void, it is first set to
> -nil.  If HOOK's value is a single function, it is changed to a
> -list of functions.
> -

Is the bit about setting HOOK to nil incorrect?  Because the new text
drops that part.

>    "Remove from the value of HOOK the function FUNCTION.
>  HOOK should be a symbol, and FUNCTION may be any valid function.  If
>  FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
> -list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
> +list of functions to run in HOOK, then nothing is done.  See `add-hook'.

"list of functions to run in HOOK" is ambiguous wrt what "in HOOK"
refers to.  I would rephrase:

  If FUNCTION is not the value of HOOK and is not a member of the list
  that is the value of HOOK, do nothing.

(This also avoids passive tense and clarifies the wording in other
aspects.)

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Tomas Nordin <tomasn@posteo.net>
>> Date: Sat, 31 Aug 2024 12:36:22 +0000
>>
>> The following on top of emacs-30.
>>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index 28ba30f584e..e60c4119c60 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -2090,6 +2090,10 @@ add-hook
>>    "Add to the value of HOOK the function FUNCTION.
>>  FUNCTION is not added if already present.
>>
>> +HOOK should be a symbol.  If HOOK is void, or if HOOK's value is a
>> +single function, it is changed to a list of functions (containing only
>> +FUNCTION in the void case).
>> +
>>  The place where the function is added depends on the DEPTH
>>  parameter.  DEPTH defaults to 0.  By convention, it should be
>>  a number between -100 and 100 where 100 means that the function
>> @@ -2108,10 +2112,6 @@ add-hook
>>  buffer-local value.  That acts as a flag to run the hook
>>  functions of the global value as well as in the local value.
>>
>> -HOOK should be a symbol.  If HOOK is void, it is first set to
>> -nil.  If HOOK's value is a single function, it is changed to a
>> -list of functions.
>> -
>
> Is the bit about setting HOOK to nil incorrect?  Because the new text
> drops that part.

It makes no difference if HOOK is "first" set to nil from the POV of the
end user, I think.  The end result is that HOOK will be a list of
functions.

IOW, the point here is that `add-hook` will work even if HOOK is void,
and that aspect is preserved in the above change.

>
>>    "Remove from the value of HOOK the function FUNCTION.
>>  HOOK should be a symbol, and FUNCTION may be any valid function.  If
>>  FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
>> -list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
>> +list of functions to run in HOOK, then nothing is done.  See `add-hook'.
>
> "list of functions to run in HOOK" is ambiguous wrt what "in HOOK"
> refers to.  I would rephrase:
>
>   If FUNCTION is not the value of HOOK and is not a member of the list
>   that is the value of HOOK, do nothing.

While the original is not ideal, I don't find this to be an improvement.
Sorry.  I'd suggest trying to reformulate it again, to make it more
direct and avoid repeating the phrase "the value of".

The wording in the manual is probably all that is needed here:

    This function removes FUNCTION from the hook variable HOOK.

In other words, we either get to assume that the user knows what a hook
is, or we can point the user to the relevant manual section to read
about it.

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Stefan Kangas]

Stefan Kangas <stefankangas@gmail.com> writes:

> The wording in the manual is probably all that is needed here:
>
>     This function removes FUNCTION from the hook variable HOOK.

BTW.  Here's another idea:

    If HOOK is a list of functions, remove FUNCTION from that list.

    If HOOK is equal to FUNCTION, set HOOK to nil.

    Otherwise do nothing.

bug#72915: Docstrings of add-hook and remove-hook improvement?

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

>> The wording in the manual is probably all that is needed here:
>>
>>     This function removes FUNCTION from the hook variable HOOK.
>
> BTW.  Here's another idea:
>
>     If HOOK is a list of functions, remove FUNCTION from that list.
>
>     If HOOK is equal to FUNCTION, set HOOK to nil.
>
>     Otherwise do nothing.

Nit pick: a hook *is* not a function or a list of functions.
It only *holds* such things.


        Stefan

bug#72915: Docstrings of add-hook and remove-hook improvement?

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

>> -HOOK should be a symbol.  If HOOK is void, it is first set to
>> -nil.  If HOOK's value is a single function, it is changed to a
>> -list of functions.
> Is the bit about setting HOOK to nil incorrect?  Because the new text
> drops that part.

That's an internal detail that's not observable to the caller anyway.

>>    "Remove from the value of HOOK the function FUNCTION.
>>  HOOK should be a symbol, and FUNCTION may be any valid function.  If
>>  FUNCTION isn't the value of HOOK, or, if FUNCTION doesn't appear in the
>> -list of hooks to run in HOOK, then nothing is done.  See `add-hook'.
>> +list of functions to run in HOOK, then nothing is done.  See `add-hook'.
>
> "list of functions to run in HOOK" is ambiguous wrt what "in HOOK"
> refers to.  I would rephrase:
>
>   If FUNCTION is not the value of HOOK and is not a member of the list
>   that is the value of HOOK, do nothing.

Maybe we can simplify the wording a bit by focusing less about whether
the hook's value is a function or a list of functions, and talking about
the "sequence" or "set" of functions (which can be represented by
a list of functions or a function)?

Something like:

      "Remove FUNCTION from HOOK's functions.
    HOOK should be a symbol, and FUNCTION may be any valid function.
    Does nothing if HOOK does not currently contain FUNCTION.
    Compares functions with `equal`, which means that it can be
    slow if FUNCTION is not a symbol.  See `add-hook'.



- Stefan

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Tomas Nordin]

Stefan Kangas <stefankangas@gmail.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> The wording in the manual is probably all that is needed here:
>>
>>     This function removes FUNCTION from the hook variable HOOK.
>
> BTW.  Here's another idea:
>
>     If HOOK is a list of functions, remove FUNCTION from that list.
>
>     If HOOK is equal to FUNCTION, set HOOK to nil.
>
>     Otherwise do nothing.

I like that, adjusted maybe to respect Stefan M's nitpick:

If HOOK's value is a list of functions, remove FUNCTION from that list.
If HOOK's value is FUNCTION, set HOOK to nil.
Otherwise do nothing.

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Stefan Kangas]

Tomas Nordin <tomasn@posteo.net> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>
>>> The wording in the manual is probably all that is needed here:
>>>
>>>     This function removes FUNCTION from the hook variable HOOK.
>>
>> BTW.  Here's another idea:
>>
>>     If HOOK is a list of functions, remove FUNCTION from that list.
>>
>>     If HOOK is equal to FUNCTION, set HOOK to nil.
>>
>>     Otherwise do nothing.
>
> I like that, adjusted maybe to respect Stefan M's nitpick:
>
> If HOOK's value is a list of functions, remove FUNCTION from that list.
> If HOOK's value is FUNCTION, set HOOK to nil.
> Otherwise do nothing.

Stefan M, does Tomas' edited version look okay to you?

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Stefan Kangas]

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

> Something like:
>
>       "Remove FUNCTION from HOOK's functions.
>     HOOK should be a symbol, and FUNCTION may be any valid function.
>     Does nothing if HOOK does not currently contain FUNCTION.
>     Compares functions with `equal`, which means that it can be
>     slow if FUNCTION is not a symbol.  See `add-hook'.

Ah, now I see that you posted this proposal.  This is a better start
than what I had.

Should we install it?

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Tomas Nordin]

Stefan Kangas <stefankangas@gmail.com> writes:

> Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Something like:
>>
>>       "Remove FUNCTION from HOOK's functions.
>>     HOOK should be a symbol, and FUNCTION may be any valid function.
>>     Does nothing if HOOK does not currently contain FUNCTION.
>>     Compares functions with `equal`, which means that it can be
>>     slow if FUNCTION is not a symbol.  See `add-hook'.
>
> Ah, now I see that you posted this proposal.  This is a better start
> than what I had.
>
> Should we install it?

This is about the doc of remove-hook. What about the edits in the
add-hook docstring, was that OK? There was a question on the bit about
setting the HOOK to nil, but I think that was sorted out.

bug#72915: Docstrings of add-hook and remove-hook improvement?

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

Tomas Nordin [2024-09-15 11:17:55] wrote:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
>> text editors" <bug-gnu-emacs@gnu.org> writes:
>>
>>> Something like:
>>>
>>>       "Remove FUNCTION from HOOK's functions.
>>>     HOOK should be a symbol, and FUNCTION may be any valid function.
>>>     Does nothing if HOOK does not currently contain FUNCTION.
>>>     Compares functions with `equal`, which means that it can be
>>>     slow if FUNCTION is not a symbol.  See `add-hook'.
>>
>> Ah, now I see that you posted this proposal.  This is a better start
>> than what I had.
>>
>> Should we install it?
>
> This is about the doc of remove-hook. What about the edits in the
> add-hook docstring, was that OK? There was a question on the bit about
> setting the HOOK to nil, but I think that was sorted out.

The `add-hook` part was OK for me, indeed.

[ I'm no great fan of that paragraph (neither the original nor the one
  you replace it with), tho, because it's a bit "too detailed" for my
  taste.  E.g. the value *always* ends up being a list of functions, and
  the parenthesis states something which sounds to me like it should be
  inferrable from the rest of the docstring.  ]


        Stefan

bug#72915: Docstrings of add-hook and remove-hook improvement?

[Tomas Nordin]

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

> The `add-hook` part was OK for me, indeed.
>
> [ I'm no great fan of that paragraph (neither the original nor the one
>   you replace it with), tho, because it's a bit "too detailed" for my
>   taste.  E.g. the value *always* ends up being a list of functions, and
>   the parenthesis states something which sounds to me like it should be
>   inferrable from the rest of the docstring.  ]

I think I agree, the parenthesized part could be discarded, shortening
the paragraph to

"HOOK should be a symbol.  If HOOK is void, or if HOOK's value is a
single function, it is changed to a list of functions."