bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all

[Stefan Monnier]

> I'm aware that I'm late to the party (got unstuck from Emacs 23
> only recently) and that there have been some reports on that
> already (bug#14734 and merged).

:-)

> But at least on one occasion Stefan has asked for a patch, and I
> haven't seen yet patches that got declined.

It does happen, tho.

> ------------------------- snip -------------------------
> sm-test1 is a Lisp macro.
>
> (sm-test1 A)
>
> <font-lock-warning-face>This function is advised.</f-l-w-f>
>
> This macro has :override advice: No documentation
>
> This is an :override advice, which means that ‘sm-test1’ isn’t
> run at all, and the documentation below may be irrelevant.
>
> This macro has :around advice: No documentation
>
> [back]
> ------------------------- snip -------------------------

Hmm... why does it say "This function is advised" here where the rest
talks about a macro instead?

[ This is not your fault, but the handling of :override isn't right
  currently, so if you could fix it while you're there, it would be
  great: the problem is that the mention of the specific override advice
  is placed at the beginning and all the others at the end, which makes
  it impossible to see the relative order of the :override advice w.r.t
  to the others.  ]

> What do you think?

I got used to the single line where I have to click to get more info, so
I'm not the target audience, but see comments below.

> +       (if (symbolp fun)
> +           (if doc
> +               (concat
> +                (format-message "`%S'\n" fun)
> +                ;; Remove first line possibly added by
> +                ;; `ad-make-single-advice-docstring' for
> +                ;; legacy advices.
> +                (if (string-match
> +                     "^\\(?:Before\\|Around\\|After\\)-advice `.*?':\n"
> +                     doc)
> +                    (substring doc (match-end 0))
> +                  doc))
> +             (format-message "`%S'." fun))

We don't want code to cater to the old `advice.el` here.
If you don't like that extra line, remove it in `advice.el` instead.
But now that `defadvice` is deprecated, I'm not sure it's worth the
trouble anyway.

The main problem I see, tho, is how to clearly "delimit" the docstring.
Maybe we should indent the advice's docstring by two spaces or so?

> +         (setq name (cdr (assq 'name (advice--props flist))))
> +         (if name
> +             (if doc
> +                 (format "%s\n%s" name doc)
> +               (format "%s" name))

[ I realize this is not your fault either, but we should say

      This function has :before advice named "NAME"

  rather than

      This function has :before advice: NAME
]

Fun situation where your code misfires:

    (defun sm-foo1 (&rest _) "Hello, this is foo1\n\nhere." nil)
    (advice-add 'diff-mode :around #'sm-foo1)
    (advice-add 'sm-foo1 :after #'ignore)

makes `C-h f diff-mode RET` show:

    [...]
    This mode runs the hook ‘diff-mode-hook’, as the final or penultimate
    step during initialization.
    
    This function is advised.
    
    This function has :around advice: ‘sm-foo1’
    Hello, this is foo1
    
    here.
    
    This function is advised.
    
    This function has :after advice: ‘ignore’
    Ignore ARGUMENTS, do nothing, and return nil.
    This function accepts any number of arguments in ARGUMENTS.
    Also see ‘always’.
    
      Probably introduced at or before Emacs version 21.1.

We can up the ante even further and advise `ignore`:

    (advice-add 'ignore :after #'sm-foo1)

such that `C-h f diff-mode RET` now tells us:

    Lisp nesting exceeds ‘max-lisp-eval-depth’

:-)

> @@ -155,10 +168,22 @@ advice--make-docstring
>        (help-add-fundoc-usage
>         (with-temp-buffer
>           (when before
> +           (insert
> +            (propertize
> +             (concat "This " (if macrop "macro" "function") " is advised.")
> +             'face 'font-lock-warning-face))
> +           (ensure-empty-lines 1)

I like this warning, but I don't like its code duplication.
The positive side is that I think this line should always come before
the main doc (and should be merged with the warning about "... override
... documentation below may be irrelevant" when applicable).


        Stefan