bug#68114: [PATCH] Make 'advice-remove' interactive

[Eli Zaretskii]

> From: Steven Allen <steven@stebalien.com>
> Cc: 68114@debbugs.gnu.org
> Date: Fri, 29 Dec 2023 12:47:50 -0800
> 
> Thanks for the quick feedback. I've attached an updated patch with a
> NEWS and update and some documentation. Let me know if there's a better
> place to document this of if I made a mistake somewhere.

Thanks, a few minor comments while we wait for Stefan to chime in.

> `ad-advice-remove' is already interactive, but it doesn't work with
> new-style advice.
> 
> * lisp/emacs-lisp/nadvice.el (advice-remove): Make it interactive.
> * doc/lispref/functions.texi (Advising Named Functions): Document that
> 'advice-remove' is now an interactive command.

Please mention the bug number, since it is now known, in the log
message.

> -@defun advice-remove symbol function
> +@deffn Command advice-remove symbol function
>  Remove the advice @var{function} from the named function @var{symbol}.
> -@var{function} can also be the @code{name} of a piece of advice.
> -@end defun
> +@var{function} can also be the @code{name} of a piece of advice. When
                                                                  ^^
Two spaces between sentences, please.

Btw, what is "the 'name' of a piece of advice"?  I realize that this
text was there to begin with, but I don't think I understand what it
wants to tell me, so maybe we could clarify that.  The only reference
to a "name" in the preceding text uses "name" to mean a symbol, but
then what is "the name of a piece of advice"?  I guess this goes back
to define-advice, which says:

  @defmac define-advice symbol (where lambda-list &optional name depth) &rest 
body
  This macro defines a piece of advice and adds it to the function named
  @var{symbol}.  The advice is an anonymous function if @var{name} is
  @code{nil} or a function named @code{symbol@@name}.  See
  @code{advice-add} for explanation of other arguments.

which is also a bit mysterious.  Does NAME used here serve as "the
name of the piece of advice"? if so, should "@code{symbol@@name}" be
"@code{symbol@@@var{name}}" instead, i.e. "name" is not a literal
string but the reference to NAME?

> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -539,6 +539,16 @@ advice-remove
>  or an autoload and it preserves `fboundp'.
>  Instead of the actual function to remove, FUNCTION can also be the `name'
>  of the piece of advice."
> +  (interactive
> +   (let ((symbol (intern (completing-read
> +                           "Advised Function: "

I wonder whether "Remove advice from function: " would be a better
prompt.

> +                           obarray
> +                           (lambda (sym) (advice--p (advice--symbol-function 
> sym)))
> +                           t nil nil
> +                           (when-let (def (function-called-at-point)) 
> (symbol-name def)))))
> +          advice)
> +     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) 
> symbol)
> +     (list symbol (cdr (assoc-string (completing-read "Advice: " advice) 
> advice)))))

And here I wonder whether "Advice to remove: " would be a better
prompt.