Package: emacs;
Reported by: Steven Allen <steven <at> stebalien.com>
Date: Fri, 29 Dec 2023 19:58:02 UTC
Severity: wishlist
Tags: patch
Done: Stefan Kangas <stefankangas <at> gmail.com>
Bug is archived. No further changes may be made.
Message #32 received at 68114 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Steven Allen <steven <at> stebalien.com> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68114 <at> debbugs.gnu.org Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive Date: Sat, 06 Jan 2024 11:02:53 +0200
> From: Steven Allen <steven <at> stebalien.com>
> Date: Sat, 30 Dec 2023 08:37:48 -0800
>
> Ah, I responded to the previous message before seeing this one.
>
> > Please mention the bug number, since it is now known, in the log
> > message.
>
> Done.
>
> > Two spaces between sentences, please.
>
> Fixed in the first revision (d4a07757).
>
> > 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?
>
> So, those two names are actually different. The 'name' in referenced in
> the `advice-remove` documentation is the 'name in the advice's 'props'
> alist. The 'name' specified in `define-advice` is _not_ added to this
> alist and is only used in the advice's function name.
>
> I'm happy to resolve this in a separate patch, if that's OK with you.
> Something like (`define-advice` documentation):
>
> Note if NAME is nil the advice is anonymous; otherwise the advice
> function is named `SYMBOL <at> NAME' and the advice is named NAME.
>
> Then actually add NAME to the properties.
>
> > I wonder whether "Remove advice from function: " would be a better
> > prompt.
>
> Good point, done!
>
> > And here I wonder whether "Advice to remove: " would be a better
> > prompt.
>
> Also done.
Stefan, any comments to the patch below?
> >From c9dbd06fd6484227e46361e39c29798750d2470e Mon Sep 17 00:00:00 2001
> From: Steven Allen <steven <at> stebalien.com>
> Date: Fri, 29 Dec 2023 09:53:05 -0800
> Subject: [PATCH] Make 'advice-remove' interactive
>
> `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 (Bug#67926).
>
> * doc/lispref/functions.texi (Advising Named Functions): Document that
> 'advice-remove' is now an interactive command.
> ---
> doc/lispref/functions.texi | 8 +++++---
> etc/NEWS | 4 ++++
> lisp/emacs-lisp/nadvice.el | 26 ++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
> index d0c8f3e90e8..6f5c1a997e2 100644
> --- a/doc/lispref/functions.texi
> +++ b/doc/lispref/functions.texi
> @@ -2077,10 +2077,12 @@ Advising Named Functions
> (@pxref{Core Advising Primitives}).
> @end defun
>
> -@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
> +called interactively, prompt for both an advised @var{function} and
> +the advice to remove.
> +@end deffn
>
> @defun advice-member-p function symbol
> Return non-@code{nil} if the advice @var{function} is already in the named
> diff --git a/etc/NEWS b/etc/NEWS
> index c002ec33d45..553365fc7a4 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -83,6 +83,10 @@ see the variable 'url-request-extra-headers'.
>
> * Changes in Emacs 30.1
>
> +** 'advice-remove' is now an interactive command.
> +When called interactively, 'advice-remove' now prompts for an advised
> +function to the advice to remove.
> +
> ** Emacs now supports Unicode Standard version 15.1.
>
> ** Network Security Manager
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index 9f2b42f5765..b1d314c0796 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -539,6 +539,32 @@ 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* ((pred (lambda (sym) (advice--p (advice--symbol-function sym))))
> + (default (when-let* ((f (function-called-at-point))
> + ((funcall pred f)))
> + (symbol-name f)))
> + (prompt (format-prompt "Remove advice from function" default))
> + (symbol (intern (completing-read prompt obarray pred t nil nil default)))
> + advices)
> + (advice-mapc (lambda (f p)
> + (let ((k (or (alist-get 'name p) f)))
> + (push (cons
> + ;; "name" (string) and 'name (symbol) are
> + ;; considered different names so we use
> + ;; `prin1-to-string' even if the name is
> + ;; a string to distinguish between these
> + ;; two cases.
> + (prin1-to-string k)
> + ;; We use `k' here instead of `f' because
> + ;; the same advice can have multiple
> + ;; names.
> + k)
> + advices)))
> + symbol)
> + (list symbol (cdr (assoc-string
> + (completing-read "Advice to remove: " advices nil t)
> + advices)))))
> (let ((f (symbol-function symbol)))
> (remove-function (cond ;This is `advice--symbol-function' but as a "place".
> ((get symbol 'advice--pending)
> --
> 2.43.0
>
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.