GNU bug report logs - #68114
[PATCH] Make 'advice-remove' interactive

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68114 in the body.
You can then email your comments to 68114 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Make 'advice-remove' interactive
Date: Fri, 29 Dec 2023 11:57:12 -0800

[Message part 1 (text/plain, inline)]

This patch makes 'advice-remove' interactive to match
'ad-advice-remove'. Unlike 'ad-advice-remove', 'advice-remove' works
with new-style advice as well.

[0001-Make-advice-remove-interactive.patch (text/x-patch, attachment)]

Message #8 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Steven Allen <steven <at> stebalien.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Fri, 29 Dec 2023 22:12:13 +0200

> From: Steven Allen <steven <at> stebalien.com>
> Date: Fri, 29 Dec 2023 11:57:12 -0800
> 
> This patch makes 'advice-remove' interactive to match
> 'ad-advice-remove'. Unlike 'ad-advice-remove', 'advice-remove' works
> with new-style advice as well.

Stefan, any comments?

In any case, this change needs a corresponding changes for
documentation and NEWS.

Thanks.

Message #11 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Fri, 29 Dec 2023 12:47:50 -0800

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/x-patch, attachment)]

Message #14 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Steven Allen <steven <at> stebalien.com>, Eli Zaretskii <eliz <at> gnu.org>, 
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Fri, 29 Dec 2023 13:20:43 -0800

Steven Allen <steven <at> stebalien.com> writes:

> --- 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
                                                                  ^^

Our coding standards mandates two spaces between sentences.

> +called interactively, prompt for both an advised @var{function} and
> +the advice to remove.
> +@end deffn


> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1362,6 +1362,10 @@ values.
>  * Lisp 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.

Doesn't this change belong under "Changes" rather than "Lisp Changes"?

It shouldn't change anything from the point of view of Lisp libraries.

Message #17 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Fri, 29 Dec 2023 14:43:59 -0800

[Message part 1 (text/plain, inline)]

Fair points, fixed.

[Message part 2 (text/x-patch, attachment)]

Message #20 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Steven Allen <steven <at> stebalien.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Sat, 30 Dec 2023 00:06:31 -0500

> --- 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: "
> +                           obarray
> +                           (lambda (sym) (advice--p (advice--symbol-function sym)))
> +                           t nil nil
> +                           (when-let (def (function-called-at-point)) (symbol-name def)))))
> +          advice)

You should include the default in the prompt using `format-prompt`.

> +     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) symbol)

The var name `advice` suggests it holds a single piece of advice.
I'd call it `advices` instead.

Also, some advice's functions are lambda expressions (i.e. closures)
which can be rather ugly/unwieldy when printed.  When code expects to
remove them, we usually provide a `name` property for them, so I suggest
that you use something like

    (lambda (f p)
       (let ((k (or (alist-get 'name p) f)))
         (push (cons (prin1-to-string k) k) advices)))

> +     (list symbol (cdr (assoc-string (completing-read "Advice: " advice) advice)))))

I suspect you want to `require-match` in the `completing-read` call.


        Stefan

Message #23 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: monnier <at> iro.umontreal.ca, 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Sat, 30 Dec 2023 08:45:11 +0200

> From: Steven Allen <steven <at> stebalien.com>
> Cc: 68114 <at> 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.

Message #26 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Sat, 30 Dec 2023 08:22:04 -0800

[Message part 1 (text/plain, inline)]

> You should include the default in the prompt using `format-prompt`.
>
>> +     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) symbol)

Fixed. I also:

- Broke the large `completing-read` call into multiple let bindings.
- Made sure that the default value was actually advised before
  suggesting it.

> The var name `advice` suggests it holds a single piece of advice.
> I'd call it `advices` instead.

Fixed

> Also, some advice's functions are lambda expressions (i.e. closures)
> which can be rather ugly/unwieldy when printed.  When code expects to
> remove them, we usually provide a `name` property for them, so I suggest
> that you use something like

Fixed, although I noticed a few oddities (commented in the code to avoid
confusing future readers).

Basically

1. 'name' can be either a string or a symbol, but they're not considered
the same name.
2. The same advice can be applied multiple times with different names,
so I changed the code to select the advice's 'name' instead of the
advice itself, if named.

> I suspect you want to `require-match` in the `completing-read` call.

Fixed.

[Message part 2 (text/x-patch, attachment)]

Message #29 received at 68114 <at> debbugs.gnu.org (full text, mbox):

From: Steven Allen <steven <at> stebalien.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Sat, 30 Dec 2023 08:37:48 -0800

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/x-patch, attachment)]

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
> 

Message #35 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: 68114 <at> debbugs.gnu.org
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Sat, 06 Jan 2024 18:48:20 +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.

I installed this patch, thanks.

> > 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.

Yes, I think something like this should be installed.

Message #42 received at 68114-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 68114-done <at> debbugs.gnu.org, Steven Allen <steven <at> stebalien.com>
Subject: Re: bug#68114: [PATCH] Make 'advice-remove' interactive
Date: Wed, 12 Feb 2025 23:05:01 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> 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.
>
> I installed this patch, thanks.

It seems like this bug was accidentally left open.

I'm closing it with this message.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.