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

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

[Steven Allen]

[-- Attachment #1: Type: text/plain, Size: 156 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-advice-remove-interactive.patch --]
[-- Type: text/x-patch, Size: 1528 bytes --]

From 41de95097d3e79967f4fbb417aba424e8ffce058 Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@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.
---
 lisp/emacs-lisp/nadvice.el | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index 9f2b42f5765..32e54b3dd02 100644
--- 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)
+     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) symbol)
+     (list symbol (cdr (assoc-string (completing-read "Advice: " advice) advice)))))
   (let ((f (symbol-function symbol)))
     (remove-function (cond ;This is `advice--symbol-function' but as a "place".
                       ((get symbol 'advice--pending)
-- 
2.43.0

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

[Eli Zaretskii]

> From: Steven Allen <steven@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.

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

[Steven Allen]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3049 bytes --]

From e1c6ccbf22a80f9ec96a7034ef66d2d7b66e793b Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@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.
* 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 | 10 ++++++++++
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index d0c8f3e90e8..30779cb7c72 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..d4c0784e3f2 100644
--- 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.
+
 ** New 'pop-up-frames' action alist entry for 'display-buffer'.
 This has the same effect as the variable of the same name and takes
 precedence over the variable when present.
diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index 9f2b42f5765..32e54b3dd02 100644
--- 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)
+     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) symbol)
+     (list symbol (cdr (assoc-string (completing-read "Advice: " advice) advice)))))
   (let ((f (symbol-function symbol)))
     (remove-function (cond ;This is `advice--symbol-function' but as a "place".
                       ((get symbol 'advice--pending)
-- 
2.43.0

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

[Stefan Kangas]

Steven Allen <steven@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.

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

[Steven Allen]

[-- Attachment #1: Type: text/plain, Size: 22 bytes --]


Fair points, fixed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2984 bytes --]

From d4a07757aea1b5618c7ee629c89ae060a285a523 Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@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.
* 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 | 10 ++++++++++
 3 files changed, 19 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'.
 \f
 * 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..32e54b3dd02 100644
--- 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)
+     (advice-mapc (lambda (f _) (push (cons (prin1-to-string f) f) advice)) symbol)
+     (list symbol (cdr (assoc-string (completing-read "Advice: " advice) advice)))))
   (let ((f (symbol-function symbol)))
     (remove-function (cond ;This is `advice--symbol-function' but as a "place".
                       ((get symbol 'advice--pending)
-- 
2.43.0

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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.

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

[Steven Allen]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3740 bytes --]

From 130adbe7542304b2fa5dd23c23d66c275cd5bcb1 Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@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.
* 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 | 24 ++++++++++++++++++++++++
 3 files changed, 33 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'.
 \f
 * 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..f2fa5f34ef8 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -539,6 +539,30 @@ 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 "Advised 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: " 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

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

[Steven Allen]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3826 bytes --]

From c9dbd06fd6484227e46361e39c29798750d2470e Mon Sep 17 00:00:00 2001
From: Steven Allen <steven@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'.
 \f
 * 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

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

[Eli Zaretskii]

> From: Steven Allen <steven@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@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@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'.
>  \f
>  * 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
> 

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

[Eli Zaretskii]

> From: Steven Allen <steven@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@NAME' and the advice is named NAME.
> 
> Then actually add NAME to the properties.

Yes, I think something like this should be installed.