Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> branch: master
> commit a30781399b3ef48150b9cb13cb0aeba7086c2d9e
> Author: Sean Whitton <spwhitton@spwhitton.name>
> Commit: Sean Whitton <spwhitton@spwhitton.name>
>
>     * subr-x (eval-command-interactive-spec): New function.
> ---
>  etc/NEWS                   | 5 +++++
>  lisp/emacs-lisp/nadvice.el | 2 ++
>  lisp/emacs-lisp/subr-x.el  | 7 +++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 9529282f047..910472e5519 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -648,6 +648,11 @@ Since circular alias chains now cannot occur, 'function-alias-p',
>  'indirect-function' and 'indirect-variable' will never signal an error.
>  Their 'noerror' arguments have no effect and are therefore obsolete.
>  
> +---
> +** New function 'eval-command-interactive-spec' in the subr-x library.
> +This function evaluates a command's interactive form and returns the
> +resultant list.

What is the motivation for this feature?

> > +
>  \f
>  * Changes in Emacs 30.1 on Non-Free Operating Systems
>  
> diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
> index e457387acc9..e6977f2012a 100644
> --- a/lisp/emacs-lisp/nadvice.el
> +++ b/lisp/emacs-lisp/nadvice.el
> @@ -165,6 +165,8 @@ DOC is a string where \"FUNCTION\" and \"OLDFUN\" are expected.")
>           (buffer-string))
>         usage))))
>  
> +;; FIXME: How about renaming this to just `eval-interactive-spec'?
> +;; It's not specific to the advice system.
>  (defun advice-eval-interactive-spec (spec)
>    "Evaluate the interactive spec SPEC."
>    (cond
> diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
> index 9e906930b92..a7d8f785508 100644
> --- a/lisp/emacs-lisp/subr-x.el
> +++ b/lisp/emacs-lisp/subr-x.el
> @@ -504,6 +504,13 @@ Used by `emacs-authors-mode' and `emacs-news-mode'."
>                            (progn (forward-line -1) (point))
>                          (point-max)))))
>  
> +(defun eval-command-interactive-spec (command)
> +  "Evaluate COMMAND's interactive form and return resultant list.
> +If COMMAND has no interactive form, return nil."
> +  (advice-eval-interactive-spec
> +   (cadr (or (and (symbolp command) (get command 'interactive-form))
> +             (interactive-form command)))))
> +
>  (provide 'subr-x)
>  
>  ;;; subr-x.el ends here

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Eli Zaretskii]

> branch: master
> commit a30781399b3ef48150b9cb13cb0aeba7086c2d9e
> Author: Sean Whitton <spwhitton@spwhitton.name>
> Commit: Sean Whitton <spwhitton@spwhitton.name>
> 
>     * subr-x (eval-command-interactive-spec): New function.
> ---
>  etc/NEWS                   | 5 +++++
>  lisp/emacs-lisp/nadvice.el | 2 ++
>  lisp/emacs-lisp/subr-x.el  | 7 +++++++
>  3 files changed, 14 insertions(+)

Thanks for working on this, but please document this new function in
the ELisp manual.  It is important enough to be mentioned there.

In the future, please always accompany user-visible changes with
suitable changes to one or both of our manuals, to make sure the
manuals are kept up-to-date at all times.

Also, the changeset touches 3 files, but only one of them is mentioned
in the log message; please avoid this in the future.  (I'm not really
sure why you needed the change to nadvice.el, FWIW.)

As for the name of the new function: what do people think about
renaming it to eval-interactive-spec?  I don't think we need the
"command" part.

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Stefan Monnier]

>     * subr-x (eval-command-interactive-spec): New function.
[...]
> +;; FIXME: How about renaming this to just `eval-interactive-spec'?
> +;; It's not specific to the advice system.

Agreed.

> +(defun eval-command-interactive-spec (command)
> +  "Evaluate COMMAND's interactive form and return resultant list.
> +If COMMAND has no interactive form, return nil."
> +  (advice-eval-interactive-spec
> +   (cadr (or (and (symbolp command) (get command 'interactive-form))
> +             (interactive-form command)))))

Why the `get` business?  If we want that, it should be in
`interactive-form`, no (and AFAICT it is there, BTW)?
And why is that preferable over `(advice-)eval-interactive-spec`?

BTW, the reason I have not defined `eval-interactive-spec` (yet?)
is because it only provides an *approximation* of what
`call-interactively` would do.

The fundamental problem in `advice-eval-interactive-spec` is that

    (defun my-foo (..)
      (interactive (advice-eval-interactive-spec 'SPEC))
      ...)

will not behave 100% the same as

    (defun my-foo (..)
      (interactive SPEC)
      ...)

for example when SPEC contains "r", because of the `visargs/varies`
business in `call-interactively`.  The difference is that with "r", the
`command-history` will contain entries like

    (my-foo ... (point) (mark) ...)

whereas with `advice-eval-interactive-spec` they'll look like:

    (my-foo ... 457 562 ...)

I'm not completely sure how we should go about faithfully exporting
`callint.c` to ELisp in such a way that it can be (re)used in
interactive forms without losing such details.

One way would be to make `eval-interactive-spec` return a pair of
arglists: a list of values (to pass to `funcall-interactively`), and
a list of expressions (to store in `command-history`).  And then allow
interactive forms to return such pairs of arglists.

Another is to make it only return a list of expressions (and use
`mapcar #'eval` when we need to get the list of values from it), and
similarly let interactive forms return lists of expressions.  But since
currently interactive forms return lists of values, we'd need to somehow
add a marker to the returned lists to distinguish if we're returning
a list of values or a list of expressions.


        Stefan

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Mon 05 Jun 2023 at 02:40PM +03, Eli Zaretskii wrote:

> Thanks for working on this, but please document this new function in
> the ELisp manual.  It is important enough to be mentioned there.
>
> In the future, please always accompany user-visible changes with
> suitable changes to one or both of our manuals, to make sure the
> manuals are kept up-to-date at all times.

I know to do this in general, but in this case the new function is in
subr-x, and my understanding was that functions in that file are not to
be documented in any manuals.  Are you suggesting I move it to subr.el?

> Also, the changeset touches 3 files, but only one of them is mentioned
> in the log message; please avoid this in the future.  (I'm not really
> sure why you needed the change to nadvice.el, FWIW.)

I should have documented the new FIXME, my apologies.

Regarding NEWS, CONTRIBUTE says

  There is no need to mention files such as NEWS and MAINTAINERS, or to
  indicate regeneration of files such as 'lib/gnulib.mk', in the
  ChangeLog entry.  "There is no need" means you don't have to, but you
  can if you want to.

Should that text perhaps be updated?

> As for the name of the new function: what do people think about
> renaming it to eval-interactive-spec?  I don't think we need the
> "command" part.

In that case, what would we rename advice-eval-interactive-spec to, when
we drop the 'advice-' prefix?

-- 
Sean Whitton

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Mon 05 Jun 2023 at 10:24AM GMT, Philip Kaludercic wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> branch: master
>> commit a30781399b3ef48150b9cb13cb0aeba7086c2d9e
>> Author: Sean Whitton <spwhitton@spwhitton.name>
>> Commit: Sean Whitton <spwhitton@spwhitton.name>
>>
>>     * subr-x (eval-command-interactive-spec): New function.
>> ---
>>  etc/NEWS                   | 5 +++++
>>  lisp/emacs-lisp/nadvice.el | 2 ++
>>  lisp/emacs-lisp/subr-x.el  | 7 +++++++
>>  3 files changed, 14 insertions(+)
>>
>> diff --git a/etc/NEWS b/etc/NEWS
>> index 9529282f047..910472e5519 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -648,6 +648,11 @@ Since circular alias chains now cannot occur, 'function-alias-p',
>>  'indirect-function' and 'indirect-variable' will never signal an error.
>>  Their 'noerror' arguments have no effect and are therefore obsolete.
>>
>> +---
>> +** New function 'eval-command-interactive-spec' in the subr-x library.
>> +This function evaluates a command's interactive form and returns the
>> +resultant list.
>
> What is the motivation for this feature?

When writing certain kinds of wrapper commands it's useful to be able to
prompt the user in just the way that an existing command does, without
reproducing that code.

-- 
Sean Whitton

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Mon 05 Jun 2023 at 12:14PM -04, Stefan Monnier wrote:

>>     * subr-x (eval-command-interactive-spec): New function.
> [...]
>> +;; FIXME: How about renaming this to just `eval-interactive-spec'?
>> +;; It's not specific to the advice system.
>
> Agreed.
>
>> +(defun eval-command-interactive-spec (command)
>> +  "Evaluate COMMAND's interactive form and return resultant list.
>> +If COMMAND has no interactive form, return nil."
>> +  (advice-eval-interactive-spec
>> +   (cadr (or (and (symbolp command) (get command 'interactive-form))
>> +             (interactive-form command)))))
>
> Why the `get` business?  If we want that, it should be in
> `interactive-form`, no (and AFAICT it is there, BTW)?

I missed that interactive-form already handles this.
I've dropped it now.

> And why is that preferable over `(advice-)eval-interactive-spec`?

You mean, why is it worth factoring out

    (advice-eval-interactive-spec (cadr (interactive-form command)))

?

Well, it's indeed less worth factoring out now that you've pointed out
my mistake about interactive-form, but it seems seems worth it to me on
balance.

-- 
Sean Whitton

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Mattias Engdegård]

> etc/NEWS                   | 5 +++++
> lisp/emacs-lisp/nadvice.el | 2 ++
> lisp/emacs-lisp/subr-x.el  | 7 +++++++
> 3 files changed, 14 insertions(+)

Mind adding a test for the new function, please?

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: emacs-devel@gnu.org
> Date: Tue, 06 Jun 2023 12:10:05 +0100
> 
> > In the future, please always accompany user-visible changes with
> > suitable changes to one or both of our manuals, to make sure the
> > manuals are kept up-to-date at all times.
> 
> I know to do this in general, but in this case the new function is in
> subr-x, and my understanding was that functions in that file are not to
> be documented in any manuals.  Are you suggesting I move it to subr.el?

My primary concern is about documenting useful features.  In which
files to put the implementation is secondary, from my POV.  We don't
need to be 110% consistent in this regard, btw.

> Regarding NEWS, CONTRIBUTE says
> 
>   There is no need to mention files such as NEWS and MAINTAINERS, or to
>   indicate regeneration of files such as 'lib/gnulib.mk', in the
>   ChangeLog entry.  "There is no need" means you don't have to, but you
>   can if you want to.
> 
> Should that text perhaps be updated?

Why? the last sentence already covers my request, doesn't it?

> > As for the name of the new function: what do people think about
> > renaming it to eval-interactive-spec?  I don't think we need the
> > "command" part.
> 
> In that case, what would we rename advice-eval-interactive-spec to, when
> we drop the 'advice-' prefix?

That function exists for quite some time, so renaming it is a much
harder decision.

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Stefan Monnier]

>> And why is that preferable over `(advice-)eval-interactive-spec`?
>
> You mean, why is it worth factoring out
>
>     (advice-eval-interactive-spec (cadr (interactive-form command)))
>
> ?

Yes.

> Well, it's indeed less worth factoring out now that you've pointed out
> my mistake about interactive-form, but it seems seems worth it to me on
> balance.

AFAICT the functionality of `advice-eval-interactive-spec` is not used
often at all, so having a specialized version for the specific case when
it's applied to (cadr (interactive-form command)) seems hard to justify.

Can you point to (potential) users?


        Stefan

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Fri 09 Jun 2023 at 10:39AM -04, Stefan Monnier wrote:

> AFAICT the functionality of `advice-eval-interactive-spec` is not used
> often at all, so having a specialized version for the specific case when
> it's applied to (cadr (interactive-form command)) seems hard to justify.
>
> Can you point to (potential) users?

Here is something I have:

    (defun spw/run-or-restore-gud (arg)
      (interactive "P")
      (if (or arg (not (and (bound-and-true-p gud-comint-buffer)
                            (get-buffer-process gud-comint-buffer))))
          ;; Start a new debugging session even if one already exists.
          ;; Killing `gud-comint-buffer' is the documented way to quit an
          ;; existing session.
          (let* ((cmd (cl-case major-mode
                        (c-mode 'gdb) (cperl-mode 'perldb) (python-mode 'pdb)
                        (t (intern (completing-read "GUD command: "
                                                    '(gdb perldb pdb) nil t)))))
                 (args (eval-command-interactive-spec cmd)))
            (when (buffer-live-p gud-comint-buffer)
              (when-let ((proc (get-buffer-process gud-comint-buffer)))
                (set-process-query-on-exit-flag proc nil))
              (kill-buffer gud-comint-buffer))
            (gdb-wait-for-pending (lambda () (apply cmd args))))
        ;; Restore the session.
        (cl-case (buffer-local-value 'gud-minor-mode gud-comint-buffer)
          (gdbmi (gdb-restore-windows)
                 ;; Try to ensure prompt is at the bottom of its window.
                 (recenter (window-body-height)))
          (t (pop-to-buffer gud-comint-buffer)))))

I initially tried implementing this using :around advice, but found that
worked less well in some respect that I can now not remember.
But I definitely tried it :)

-- 
Sean Whitton

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Tue 06 Jun 2023 at 02:59PM +03, Eli Zaretskii wrote:

> My primary concern is about documenting useful features.  In which
> files to put the implementation is secondary, from my POV.  We don't
> need to be 110% consistent in this regard, btw.

Okay.  I'll add it to the manual if we decide we're keeping it.

>> Regarding NEWS, CONTRIBUTE says
>>
>>   There is no need to mention files such as NEWS and MAINTAINERS, or to
>>   indicate regeneration of files such as 'lib/gnulib.mk', in the
>>   ChangeLog entry.  "There is no need" means you don't have to, but you
>>   can if you want to.
>>
>> Should that text perhaps be updated?
>
> Why? the last sentence already covers my request, doesn't it?

It seemed to me that the text should say that it's considered preferable
to mention them.  The current text leaves it wholly up to an
individual's inclination.

>> > As for the name of the new function: what do people think about
>> > renaming it to eval-interactive-spec?  I don't think we need the
>> > "command" part.
>>
>> In that case, what would we rename advice-eval-interactive-spec to, when
>> we drop the 'advice-' prefix?
>
> That function exists for quite some time, so renaming it is a much
> harder decision.

If we decide not to rename it, wouldn't it then be confusing to have
advice-eval-interactive-spec and eval-interactive-spec taking different
kinds of argument, with no indication in their name about the difference?

-- 
Sean Whitton

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: emacs-devel@gnu.org
> Date: Wed, 14 Jun 2023 11:18:20 +0100
> 
> >>   There is no need to mention files such as NEWS and MAINTAINERS, or to
> >>   indicate regeneration of files such as 'lib/gnulib.mk', in the
> >>   ChangeLog entry.  "There is no need" means you don't have to, but you
> >>   can if you want to.
> >>
> >> Should that text perhaps be updated?
> >
> > Why? the last sentence already covers my request, doesn't it?
> 
> It seemed to me that the text should say that it's considered preferable
> to mention them.  The current text leaves it wholly up to an
> individual's inclination.

Which is basically the truth.  I personally prefer to mention every
file, but I don't feel like making my preferences codified and carved
in stone.

> >> > As for the name of the new function: what do people think about
> >> > renaming it to eval-interactive-spec?  I don't think we need the
> >> > "command" part.
> >>
> >> In that case, what would we rename advice-eval-interactive-spec to, when
> >> we drop the 'advice-' prefix?
> >
> > That function exists for quite some time, so renaming it is a much
> > harder decision.
> 
> If we decide not to rename it, wouldn't it then be confusing to have
> advice-eval-interactive-spec and eval-interactive-spec taking different
> kinds of argument, with no indication in their name about the difference?

We use aliases to prevent such confusion.

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Stefan Monnier]

>> AFAICT the functionality of `advice-eval-interactive-spec` is not used
>> often at all, so having a specialized version for the specific case when
>> it's applied to (cadr (interactive-form command)) seems hard to justify.
>>
>> Can you point to (potential) users?
>
> Here is something I have:
>
>     (defun spw/run-or-restore-gud (arg)
>       (interactive "P")
>       (if (or arg (not (and (bound-and-true-p gud-comint-buffer)
>                             (get-buffer-process gud-comint-buffer))))
>           ;; Start a new debugging session even if one already exists.
>           ;; Killing `gud-comint-buffer' is the documented way to quit an
>           ;; existing session.
>           (let* ((cmd (cl-case major-mode
>                         (c-mode 'gdb) (cperl-mode 'perldb) (python-mode 'pdb)
>                         (t (intern (completing-read "GUD command: "
>                                                     '(gdb perldb pdb) nil t)))))
>                  (args (eval-command-interactive-spec cmd)))
>             (when (buffer-live-p gud-comint-buffer)
>               (when-let ((proc (get-buffer-process gud-comint-buffer)))
>                 (set-process-query-on-exit-flag proc nil))
>               (kill-buffer gud-comint-buffer))
>             (gdb-wait-for-pending (lambda () (apply cmd args))))
>         ;; Restore the session.
>         (cl-case (buffer-local-value 'gud-minor-mode gud-comint-buffer)
>           (gdbmi (gdb-restore-windows)
>                  ;; Try to ensure prompt is at the bottom of its window.
>                  (recenter (window-body-height)))
>           (t (pop-to-buffer gud-comint-buffer)))))
>
> I initially tried implementing this using :around advice, but found that
> worked less well in some respect that I can now not remember.
> But I definitely tried it :)

OK, so that's one example.  Any other?  Because, to me, a single
use-case doesn't seem to justify the definition of a one-liner helper
function.
[ Especially since that helper function is still not a faithful
  simulation of what happens in a "normal" execution of the command
  and fixing that wrinkle will likely require changing the API :-(  ]


        Stefan

Re: master a30781399b3: * subr-x (eval-command-interactive-spec): New function.

[Sean Whitton]

Hello,

On Wed 14 Jun 2023 at 04:52PM -04, Stefan Monnier wrote:

> OK, so that's one example.  Any other?  Because, to me, a single
> use-case doesn't seem to justify the definition of a one-liner helper
> function.
> [ Especially since that helper function is still not a faithful
>   simulation of what happens in a "normal" execution of the command
>   and fixing that wrinkle will likely require changing the API :-(  ]

Right.  I don't have another example yet, and I don't think I would have
proposed adding this at all had I realised that most of it is already
implemented in `interactive-form'.  If no-one else speaks up in favour
of keeping it, I'll go ahead and remove it again.

-- 
Sean Whitton