bug#59379: 29.0.50; `define-advice' documentation needs improving

[Visuwesh]

[சனி நவம்பர் 19, 2022] Stefan Kangas wrote:

> Visuwesh <visuweshm@gmail.com> writes:
>
>>> 3. This is its argument list:
>>>
>>>    (define-advice SYMBOL (HOW LAMBDA-LIST &optional NAME DEPTH) &rest
>>>    BODY)
>>>
>>>    The HOW, LAMBDA-LIST, NAME, DEPTH parameters are not documented in
>>>    the docstring, nor in the info manual.
>>
>> HOW, LAMBDA-LIST, NAME, and DEPTH arguments become clear when once looks
>> up the add-function docstring, and the docstring already mentions
>> add-function.
>
> Then that should be explicitly stated.

Is that not already spelt out?

    Define an advice and add it to function named SYMBOL.
    See ‘advice-add’ and ‘add-function’ for explanation on the
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    arguments.  Note if NAME is nil the advice is anonymous;
    ^^^^^^^^^^
    otherwise it is named ‘SYMBOL@NAME’.

> And LAMBDA-LIST is not explained there either, AFAICT.

Reading the advice-add's document should tell what it means but I might
be biased here.

>>> 5. The documentation of NAME says that: "The advice is an anonymous
>>>    function if NAME is ‘nil’ or a function named ‘symbol@name’."
>>>
>>>    I struggle with parsing this sentence.  It sounds like it is saying
>>>    that, if I want an anonymous function, I should define a function
>>>    named `symbol@name' (substituting `symbol' and `name') and then pass
>>>    that argument as the NAME argument?  But then the function is not
>>>    anonymous?
>>
>> Would a comma help before the "or"? i.e.,
>>
>>     The advice is an anonymous function if NAME is ‘nil’, or a function
>>     named ‘symbol@name’.
>
> So it can be either nil or a symbol?  How do I actually use it?

Yes.  See below for an example with a non-nil NAME

    (define-advice file-cache-file-name (:filter-return (filename) 
vz/add-slash-if-directory)
      "Add a trailing slash if FILENAME is a directory."
      (if (file-directory-p filename)
          (concat filename "/")
        filename))

which creates a function file-cache-file-name@vz/add-slash-if-directory.
But if NAME was nil, then there would be no named function but rather a
lambda that gets added as an advice (which is hard to remove later).

>> Changing symbol@name to SYMBOL@NAME like in the docstring will make it
>> clearer, I think.  If still not clear, the following happens in the case
>> of NAME being nil vs. non-nil
>>
>>     NAME nil ==> (advice-add SYMBOL HOW (lambda LAMBDA-LIST BODY) ...)
>>     NAME non-nil ==> (advice-add SYMBOL HOW (defun SYMBOL@NAME LAMBDA-LIST 
>> BODY) ...)
>
> This all needs to be explained clearly in the documentation.

Again, it seems obvious to me.