Re: Add completion to compilation-read-command

[Philip Kaludercic <philipk@posteo.net>]

Spyros Roum <spyros.roum@posteo.net> writes:

> Philip Kaludercic wrote:
>>> Also, I'm wondering if this counts as non-trivial enough that I would
>>> need to sign the FSF copyright.
>> It looks like it to me, and either way it is not bad to do that so that
>> you have it behind you for future contributions either to Emacs or ELPA.
> You are right, I have initiated the process.
>>> Regarding testing, as far as I can tell, there are currently no tests
>>> for `compilation-read-command`.
>>> Should I add anything? If yes, I'll probably need someone to point to
>>> existing tests for similar things that I can copy from.
>> You could take a look at test/lisp/progmodes/compile-tests.el, but I
>> don't think this breaks the patch, especially when dealing with
>> something as interactive as what you are proposing.
>
> I did check compile-tests.el and found no references to
> `compilation-read-command`,
> that's why I said I think it's not tested currently.

Ah my bad, I misunderstood that you couldn't find any tests at all.

> I'll not add any tests, I wouldn't even know how to test something
> interactive like that.

IIRC you could simulate input using `unread-command-events'.

>>> Am I forgetting anything else?
>>>  From e1068206662913978d541f924205a0615f8d2d95 Mon Sep 17 00:00:00 2001
>>> From: Spyros Roum<spyros.roum@posteo.net>
>>> Date: Wed, 25 Dec 2024 17:32:31 +0200
>>> Subject: [PATCH] Add option for making compilation-read-command use
>>>   completing-read
>> A ChageLog style commit message would be expected here as well.  You can
>> inject the structure in vc-mode using C-c C-w.
>
> I'm not sure the level of detail I should go in describing each
> function I touch, so I wrote (what I think is) a lot. Let me know if
> it's not necessary or if it's fine as is. I did see some examples from
> the wiki/other commits and I think it's fine.
>
>>> +  "Function used by `compilation-read-command' to get user's input.
>>> +Defaults to `compilation-prompt-read-shell-command',
>>> +but `compilation-prompt-read-command-with-completion' can be used instead for
>> If possible, avoid the passive phrase here.
>
> I changed it to active voice. I'll also say I'm not a native speaker,
> so if you think
> it's still weird/not good enough, please let me know.
>
> I've attached a new draft based on your feedback. Additionally, I've
> renamed the new function to something that describes that completion
> happens based on history, according to Juri's input.
>
> One additional question: The NEWS file is for version 31.1. Should the
> `:version` tag also be for 31.1 instead of 31?

It should be for 31.1, as that will be the version when Emacs is released.

> From 7e140c1ab5bfc7440753ab3aff2c3ce7eb38414e Mon Sep 17 00:00:00 2001
> From: Spyros Roum <spyros.roum@posteo.net>
> Date: Wed, 25 Dec 2024 17:32:31 +0200
> Subject: [PATCH] Add option for making compilation-read-command use
>  completing-read
>
> * etc/NEWS: Add to NEWS
> * lisp/progmodes/compile.el (compilation-read-command):
> Call function based on `compilation-read-command-function`.
>
> (compilation-prompt-read-shell-command): The existing
> functionality from compilation-read-command extracted to
> a function.
>
> (compilation-prompt-read-with-history-completion): A
> function that uses completing-read to read the command.
>
> (compilation-read-command-function): The new option that
> controlls which function is used.

The detail seems fine, the formatting is just unusual but that can be
fixed when applying the patch.

> ---
>  etc/NEWS                  |  8 ++++++++
>  lisp/progmodes/compile.el | 28 +++++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index ca107bb4938..cfb137c2399 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -147,6 +147,14 @@ will still be on that candidate after "*Completions*" is updated with a
>  new list of completions.  The candidate is automatically deselected when
>  the "*Completions*" buffer is hidden.
>  
> +---
> +*** New user option 'compilation-read-command-function'.
> +This option controls what function is used to read user input for
> +'compilation-read-command'.
> +It defaults to 'compilation-prompt-read-shell-command', which preserves
> +existing behavior.  When set to 'compilation-prompt-read-with-history-completion',
> +'completing-read' is used allowing autocomplete based on past runs of 'compile'.

I am not sure if we need to go into the options here.

> +
>  ** Windows
>  
>  +++
> diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
> index 6784a12fd63..d01e8c69017 100644
> --- a/lisp/progmodes/compile.el
> +++ b/lisp/progmodes/compile.el
> @@ -1797,12 +1797,38 @@ compilation-mode-font-lock-keywords
>     '((compilation--ensure-parse))
>     compilation-mode-font-lock-keywords))
>  
> -(defun compilation-read-command (command)
> +(defun compilation-prompt-read-shell-command (command)
>    (read-shell-command "Compile command: " command
>                        (if (equal (car compile-history) command)
>                            '(compile-history . 1)
>                          'compile-history)))
>  
> +(defun compilation-prompt-read-with-history-completion (command)
> +  (completing-read "Compile command: " compile-history
> +                   nil nil command
> +                   (if (equal (car compile-history) command)
> +                       '(compile-history . 1)
> +                     'compile-history)))
> +
> +(defcustom compilation-read-command-function
> +  #'compilation-prompt-read-shell-command
> +  "`compilation-read-command' uses this function to get user's input.
> +Defaults to `compilation-prompt-read-shell-command',
> +but 'compilation-prompt-read-with-history-completion' can be used instead for
> +a completing version based on past runs."
> +  :version "31.0"
> +  :type 'function
> +  :options
> +  (list
> +    #'compilation-prompt-read-shell-command
> +    #'compilation-prompt-read-with-history-completion))
> +
> +(defun compilation-read-command (command)
> +  "Prompt user for command to run.
> +`compilation-read-command-function' controls the way input is read
> +from the minibuffer."
> +  (funcall compilation-read-command-function command))
> +
>  \f
>  ;;;###autoload
>  (defun compile (command &optional comint)