GNU bug report logs - #57673
[PATCH] Parse --help messages for pcomplete

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 57673 in the body.
You can then email your comments to 57673 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: Augusto Stoffel <arstoffel <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 11:34:31 +0200

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

Tags: patch

Find attached a “worse is better” approach for pcomplete, where we parse
help messages to generate a list of completions.

This is still a sketch.  I've added pcomplete functions for a random
selection of commands to see if this works reasonably, and I think it
probably does.  But in any case I don't think it would make sense to try
and add completions as detailed as the ones bash provides; there is an
awful lot of logic in the files under /usr/share/bash-completion and I
don't think anyone would want to redo that work.

Some further comments:

1. I'm a bit unsure whether `pcomplete-parse-help' should return a plain
   list of completions or a completion table.  The advantage of the
   former (which is the current approach) is that it's easier to
   manipulate the result of the parsing (cf. the need for that in the
   git case).  The advantage of returning a completion table is a better
   treatment of annotations.

2. I would rather not create a new pcmpl-git.el file (doing so for each
   new command doesn't seem very scalable), but I wouldn't know where
   else to put that stuff.

[0001-Add-pcomplete-parse-help.patch (text/patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 14:39:22 +0200

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> Find attached a “worse is better” approach for pcomplete, where we parse
> help messages to generate a list of completions.

Hm, interesting.  Perhaps Stefan has comments here; added to the CCs.

> This is still a sketch.  I've added pcomplete functions for a random
> selection of commands to see if this works reasonably, and I think it
> probably does.  But in any case I don't think it would make sense to try
> and add completions as detailed as the ones bash provides; there is an
> awful lot of logic in the files under /usr/share/bash-completion and I
> don't think anyone would want to redo that work.

I'm wholly unfamiliar with how bash does completions.  But is there any
reasonable way to reuse the bash completion framework here?

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 15:05:22 +0200

On Thu,  8 Sep 2022 at 14:39, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:

> I'm wholly unfamiliar with how bash does completions.  But is there any
> reasonable way to reuse the bash completion framework here?

There is a MELPA package that does that:

    https://github.com/szermatt/emacs-bash-completion

But I it never worked very well for me, possibly because I often use
Tramp connections with high latency.  IIUC this package is like Python's
"native completion", i.e., it relies on sending "prefix\t\t" to the bash
process and hopes readline will do its thing.

Exploiting bash could work nicely if it had a
please_complete_this_partial_command function, but this doesn't seem
possible.  I might be wrong, though.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 16:49:03 -0400

> +;;; Commentary:
> +

I don't fully agree with you here.

> +(defun pcmpl-git--tracked-file-predicate ()
> +  "Return a predicate function determining if a file is tracked by git."

I'd capitalize "git".

> +  (when-let ((files (ignore-errors
> +                      (process-lines vc-git-program "ls-files")))

Is it normal&common for `ls-files` to return an error?  If not, then
maybe we should use `with-demoted-errors` so that the users are made
aware of an error if it occurs.

> +(defun pcmpl-git--remote-refs (remote)
> +  "List the locally known git revisions from REMOTE.
> +If REMOTE is nil, return the list of remotes."
> +  (if (null remote)

AFAICT you could just as well have two separate functions here since all
callers either always provide a nil arg or never provide a nil arg.

> +      (ignore-errors
> +        (process-lines vc-git-program "remote"))
> +    (delq nil
> +          (mapcar
> +           (let ((re (concat (regexp-quote remote) "/\\(.*\\)")))
> +             (lambda (s) (when (string-match re s) (match-string 1 s))))
> +           (vc-git-revision-table nil)))))

I think the `re` needs to be anchored to avoid false positives.

> +;;;###autoload
> +(defun pcomplete/git ()
> +  "Completion for the `git' command."
> +  (let ((subcmds (pcomplete-parse-help "git help -a"
> +                                       :margin "^\\( +\\)[a-z]"
> +                                       :argument "[-a-z]+")))
> +    (while (not (member (pcomplete-arg 1) subcmds))
> +      (pcomplete-here (append subcmds
> +                              (pcomplete-parse-help "git help"
> +                                                    :margin "\\(\\[\\)-"
> +                                                    :separator " | "
> +                                                    :description "\\`"))))

I don't quite understand this `while` loop.  IIUC this is to handle the
case where `--foo` args are passed before the `subcmd`, but if we want
to support that use case, don't we need to change the code so it doesn't
hardcode the `1` in (pcomplete-arg 1)?

Maybe we should follow a scheme similar to that used in `pcomplete-cvs`,
tho it'd probably require a new replacement for `pcomplete-opt`

> +    (let ((subcmd (pcomplete-arg 1)))
> +      (while (pcase subcmd
> +               ((guard (pcomplete-match "\\`-" 0))
> +                (pcomplete-here
> +                 (pcmpl-git--expand-flags
> +                  (pcomplete-parse-help (format "git help %s" subcmd)
> +                                        :argument "-+\\(?:\\[no-\\]\\)?[a-z-]+=?"))))

`subcmd` may contain funny chars like `&`, so we should
`shell-quote-argument` it (tho see further down).

> +               ;; Complete tracked files
> +               ((or "mv" "rm" "restore" "grep" "status" "commit")
> +                (pcomplete-here
> +                 (pcomplete-entries nil (pcmpl-git--tracked-file-predicate))))

Regarding `git commit`:
- I never specify files on `git commit` without using `--` first.
- I often appreciate the completion of `--amend`.
- It probably makes sense to only complete files that have been modified.

> +(cl-defun pcomplete-parse-help (command

AFAICT, this "command" never uses any functionality of the shell, so we
should be using `call-process` rather than `call-process-shell-command`,
since the use of a shell here only brings trouble (it's less efficient
and it forces us to `shell-quote-arguments` to try and avoid
pathological errors in corner cases).


        Stefan

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 23:53:02 +0200

Hi Stefan,

thanks for the detailed comments.  I was also curious, on a more high
level, about your opinion on parsing the --help messages.  It's a bit of
a heuristic thing and it will probably lead to some false positives here
and there.

On Thu,  8 Sep 2022 at 16:49, Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> +;;; Commentary:
>> +
>
> I don't fully agree with you here.

I thought this was pretty uncontroversial, but I don't feel too strongly
about it.

>> +(defun pcmpl-git--tracked-file-predicate ()
>> +  "Return a predicate function determining if a file is tracked by git."
>
> I'd capitalize "git".

Noted.

>> +  (when-let ((files (ignore-errors
>> +                      (process-lines vc-git-program "ls-files")))
>
> Is it normal&common for `ls-files` to return an error?  If not, then
> maybe we should use `with-demoted-errors` so that the users are made
> aware of an error if it occurs.

I think that being outside of a repo is the main error situation.  But
in this particular spot I'd rather fail silently, because the
consequence of an error is rather innocuous -- we'll lack a predicate
function and therefore show some extra files completions.

>> +(defun pcmpl-git--remote-refs (remote)
>> +  "List the locally known git revisions from REMOTE.
>> +If REMOTE is nil, return the list of remotes."
>> +  (if (null remote)
>
> AFAICT you could just as well have two separate functions here since all
> callers either always provide a nil arg or never provide a nil arg.

That's true.  I was trying to save a symbol name.

>> +      (ignore-errors
>> +        (process-lines vc-git-program "remote"))
>> +    (delq nil
>> +          (mapcar
>> +           (let ((re (concat (regexp-quote remote) "/\\(.*\\)")))
>> +             (lambda (s) (when (string-match re s) (match-string 1 s))))
>> +           (vc-git-revision-table nil)))))
>
> I think the `re` needs to be anchored to avoid false positives.

Good point.

>> +;;;###autoload
>> +(defun pcomplete/git ()
>> +  "Completion for the `git' command."
>> +  (let ((subcmds (pcomplete-parse-help "git help -a"
>> +                                       :margin "^\\( +\\)[a-z]"
>> +                                       :argument "[-a-z]+")))
>> +    (while (not (member (pcomplete-arg 1) subcmds))
>> +      (pcomplete-here (append subcmds
>> +                              (pcomplete-parse-help "git help"
>> +                                                    :margin "\\(\\[\\)-"
>> +                                                    :separator " | "
>> +                                                    :description "\\`"))))
>
> I don't quite understand this `while` loop.  IIUC this is to handle the
> case where `--foo` args are passed before the `subcmd`, but if we want
> to support that use case, don't we need to change the code so it doesn't
> hardcode the `1` in (pcomplete-arg 1)?

The thing here is that each call to `pcomplete-here' shifts the args.
So `1' in this context means “the previous arg”.  In English, the above
form means: offer global switches and subcommand names as completion
candidates until the previous arg is a subcommand.

(We could additionally allow filenames, since some switches take a file
argument.)

> Maybe we should follow a scheme similar to that used in `pcomplete-cvs`,
> tho it'd probably require a new replacement for `pcomplete-opt`

I don't understand what you mean exactly here, but note that pcomplete
allows entering, for instance

    git --no-pager -C commit --amend --long ./m4/acl.m4

without completely typing any of these words.

>> +    (let ((subcmd (pcomplete-arg 1)))
>> +      (while (pcase subcmd
>> +               ((guard (pcomplete-match "\\`-" 0))
>> +                (pcomplete-here
>> +                 (pcmpl-git--expand-flags
>> +                  (pcomplete-parse-help (format "git help %s" subcmd)
>> +                                        :argument "-+\\(?:\\[no-\\]\\)?[a-z-]+=?"))))
>
> `subcmd` may contain funny chars like `&`, so we should
> `shell-quote-argument` it (tho see further down).

True, we shouldn't use a shell.  But also note that a funny char will
not appear in an element of `subcmds', since our :argument parameter is
"[-a-z]+".

>> +               ;; Complete tracked files
>> +               ((or "mv" "rm" "restore" "grep" "status" "commit")
>> +                (pcomplete-here
>> +                 (pcomplete-entries nil (pcmpl-git--tracked-file-predicate))))
>
> Regarding `git commit`:
> - I never specify files on `git commit` without using `--` first.

Well, `--' is among the completion candidates.  Is that what you want?

> - I often appreciate the completion of `--amend`.

You can complete switches after `git commit'.  This is taken care of by
the (guard (pcomplete-match "\\`-" 0)) case of the pcase.

> - It probably makes sense to only complete files that have been modified.

So, this is brings up an important point.

I'm advocating for a “worse is better” method where we try to have a
more or less comprehensive list of completion candidates but we don't
try to be too precise about the context in which each thing can appear.

If we do try to be precise, we might end up with 3592 lines of code,
which is the length of /usr/share/bash-completion/git on my machine.

What do you think?

>> +(cl-defun pcomplete-parse-help (command
>
> AFAICT, this "command" never uses any functionality of the shell, so we
> should be using `call-process` rather than `call-process-shell-command`,
> since the use of a shell here only brings trouble (it's less efficient
> and it forces us to `shell-quote-arguments` to try and avoid
> pathological errors in corner cases).

Yes, passing the output of `format' straight to a shell is of course not
acceptable for the final version of this patch.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Thu, 08 Sep 2022 22:47:26 -0400

Augusto Stoffel [2022-09-08 23:53:02] wrote:
> thanks for the detailed comments.  I was also curious, on a more high
> level, about your opinion on parsing the --help messages.  It's a bit of
> a heuristic thing and it will probably lead to some false positives here
> and there.

I'm all for using `--help`.

I think any problems should be pushed upstream: the commands should be
self-describing so if `--help` can't be used reliably, they should
provide something else that can.

>>> +;;; Commentary:
>>> +
>> I don't fully agree with you here.
> I thought this was pretty uncontroversial, but I don't feel too strongly
> about it.

[ OK, you got me at my own game: I can't tell if you're pushing the
  joke further or if you're serious :-)  ]

>>> +  (when-let ((files (ignore-errors
>>> +                      (process-lines vc-git-program "ls-files")))
>>
>> Is it normal&common for `ls-files` to return an error?  If not, then
>> maybe we should use `with-demoted-errors` so that the users are made
>> aware of an error if it occurs.
>
> I think that being outside of a repo is the main error situation.  But
> in this particular spot I'd rather fail silently, because the
> consequence of an error is rather innocuous -- we'll lack a predicate
> function and therefore show some extra files completions.

Fair, enough, tho being outside of a repo should be rather unusual, no
(unless Emacs's dir tracker has gone hooftracks, admittedly).
In any case, you might want to add a brief comment about why you decided
to use `ignore-errors`.

>>> +;;;###autoload
>>> +(defun pcomplete/git ()
>>> +  "Completion for the `git' command."
>>> +  (let ((subcmds (pcomplete-parse-help "git help -a"
>>> +                                       :margin "^\\( +\\)[a-z]"
>>> +                                       :argument "[-a-z]+")))
>>> +    (while (not (member (pcomplete-arg 1) subcmds))
>>> +      (pcomplete-here (append subcmds
>>> +                              (pcomplete-parse-help "git help"
>>> +                                                    :margin "\\(\\[\\)-"
>>> +                                                    :separator " | "
>>> +                                                    :description "\\`"))))
>>
>> I don't quite understand this `while` loop.  IIUC this is to handle the
>> case where `--foo` args are passed before the `subcmd`, but if we want
>> to support that use case, don't we need to change the code so it doesn't
>> hardcode the `1` in (pcomplete-arg 1)?
>
> The thing here is that each call to `pcomplete-here' shifts the args.
> So `1' in this context means “the previous arg”.

Oh, right it's not the absolute position, now I get it, thanks.
Looks good, then.

>>> +               ;; Complete tracked files
>>> +               ((or "mv" "rm" "restore" "grep" "status" "commit")
>>> +                (pcomplete-here
>>> +                 (pcomplete-entries nil (pcmpl-git--tracked-file-predicate))))
>>
>> Regarding `git commit`:
>> - I never specify files on `git commit` without using `--` first.
>
> Well, `--' is among the completion candidates.  Is that what you want?

No, I'm just not sure we should be completing file names before a "--"
was given.  Maybe it's OK, tho.

>> - I often appreciate the completion of `--amend`.
> You can complete switches after `git commit'.  This is taken care of by
> the (guard (pcomplete-match "\\`-" 0)) case of the pcase.

Indeed, I missed that.

>> - It probably makes sense to only complete files that have been modified.
> So, this is brings up an important point.
> I'm advocating for a “worse is better” method where we try to have a
> more or less comprehensive list of completion candidates but we don't
> try to be too precise about the context in which each thing can appear.

Fair enough.  Especially since being more precise risks missing some
files which can legitimately appear.


        Stefan

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Fri, 09 Sep 2022 19:02:27 +0200

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> IIUC this package is like Python's
> "native completion", i.e., it relies on sending "prefix\t\t" to the bash
> process and hopes readline will do its thing.

I see.

> Exploiting bash could work nicely if it had a
> please_complete_this_partial_command function, but this doesn't seem
> possible.  I might be wrong, though.

I wondered whether we could use those files more directly -- executing
them or something in the right context?  (Like I said, I know nothing
about how this works. 😀)

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Sat, 10 Sep 2022 11:20:28 +0200

On Fri,  9 Sep 2022 at 19:02, Lars Ingebrigtsen <larsi <at> gnus.org> wrote:

> Augusto Stoffel <arstoffel <at> gmail.com> writes:
>
>> IIUC this package is like Python's
>> "native completion", i.e., it relies on sending "prefix\t\t" to the bash
>> process and hopes readline will do its thing.
>
> I see.
>
>> Exploiting bash could work nicely if it had a
>> please_complete_this_partial_command function, but this doesn't seem
>> possible.  I might be wrong, though.
>
> I wondered whether we could use those files more directly -- executing
> them or something in the right context?  (Like I said, I know nothing
> about how this works. 😀)

Yeah, I'm not sure either, but I would suspect the MELPA package
mentioned above didn't choose the hardest path gratuitously :-).  It
would be good to hear from someone who knows for sure, though.

One additional advantage of parsing the help files which I didn't
mention before is that we get a modicum of self-documenting behavior,
since we can collect the argument descriptions.  For example, while
completing you can remind yourself which switch is for recursive, -r or
-R.

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Sat, 10 Sep 2022 11:45:08 +0200

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

Hi Stefan,

I've attached a new iteration of the patch.  I think the git completion
should be pretty usable (but certainly can be refined in the future).
I'm also satisfied with the parser, please have a look if you are
interested.

Next I'd like to think now of a good way to add batches of simpler
commands, say all GNU coreutils.  This would entail repeating variations
of

   (defun pcomplete/gpg ()
     "Completion for the GNU Privacy Guard."
     (while (if (pcomplete-match "\\`-" 0)
                (pcomplete-here (pcomplete-from-help "gpg --help"
                                                     :narrow-end "^ -se"))
              (pcomplete-here (pcomplete-entries)))))

over and over, so I was wondering if it makes sense to add a macro to
help here.  See a suggestion at the end of pcomplete.el.

[0001-pcomplete-Generate-completions-from-help-messages.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Sat, 10 Sep 2022 10:32:38 -0400

> I've attached a new iteration of the patch.  I think the git completion
> should be pretty usable (but certainly can be refined in the future).
> I'm also satisfied with the parser, please have a look if you are
> interested.

LGTM, feel free to push it to `master`.

> Next I'd like to think now of a good way to add batches of simpler
> commands, say all GNU coreutils.  This would entail repeating variations
> of
>
>    (defun pcomplete/gpg ()
>      "Completion for the GNU Privacy Guard."
>      (while (if (pcomplete-match "\\`-" 0)
>                 (pcomplete-here (pcomplete-from-help "gpg --help"
>                                                      :narrow-end "^ -se"))
>               (pcomplete-here (pcomplete-entries)))))
>
> over and over, so I was wondering if it makes sense to add a macro to
> help here.  See a suggestion at the end of pcomplete.el.

I do think it makes sense, but I think it doesn't need to be a macro:

> +;; What do you think of a macro like this?
> +(defmacro define-simple-pcomplete (name command &rest args)
> +  "Create `pcomplete' completions for a simple command.
> +COMMAND and ARGS are as in `pcomplete-from-help'.  Completion
> +candidates for this command will include the parsed arguments as
> +well as files."
> +  (let* ((namestr (symbol-name name))
> +         (docstring (if-let ((i (string-search "/" namestr)))
> +                       (format "Completions for the `%s' command in `%s'."
> +                               (substring namestr 0 i)
> +                               (substring namestr i))
> +                     (format "Completions for the `%s' command." namestr))))
> +    `(defun ,(intern (concat "pcomplete/" namestr)) ()
> +       ,docstring
> +       (pcomplete--simple-command ,command ',args))))

    (defun define-simple-pcomplete (name command &rest args)
      "Create `pcomplete' completions for a simple command.
    COMMAND and ARGS are as in `pcomplete-from-help'.  Completion
    candidates for this command will include the parsed arguments as
    well as files."
      (let* ((namestr (symbol-name name))
             (docstring (if-let ((i (string-search "/" namestr)))
                           (format "Completions for the `%s' command in `%s'."
                                   (substring namestr 0 i)
                                   (substring namestr i))
                         (format "Completions for the `%s' command." namestr))))
        (defalias (intern (concat "pcomplete/" namestr))
          (lambda ()
           (:documentation docstring)
           (pcomplete--simple-command command args)))))

Also, we may end up with various "simple" solutions, so I'd use a name
that is more explicit about how it works.  E.g. `pcomplete-via-help`?


        Stefan

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Sat, 10 Sep 2022 18:12:31 +0200

On Sat, 10 Sep 2022 at 10:32, Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> I've attached a new iteration of the patch.  I think the git completion
>> should be pretty usable (but certainly can be refined in the future).
>> I'm also satisfied with the parser, please have a look if you are
>> interested.
>
> LGTM, feel free to push it to `master`.

Sounds good, I'll send a mergeable patch in due time.

>> Next I'd like to think now of a good way to add batches of simpler
>> commands, say all GNU coreutils.  This would entail repeating variations
>> of
>>
>>    (defun pcomplete/gpg ()
>>      "Completion for the GNU Privacy Guard."
>>      (while (if (pcomplete-match "\\`-" 0)
>>                 (pcomplete-here (pcomplete-from-help "gpg --help"
>>                                                      :narrow-end "^ -se"))
>>               (pcomplete-here (pcomplete-entries)))))
>>
>> over and over, so I was wondering if it makes sense to add a macro to
>> help here.  See a suggestion at the end of pcomplete.el.
>
> I do think it makes sense, but I think it doesn't need to be a macro:

Yes, the function is wholly sufficient, except that I don't know how to
make the definitions autoloadable.

I guess there's no performance reasons to autoload these little
definitions, but we would need autoloading to keep the grouping of
commands into the different pcmpl-*.el libraries.

So how to I teach the autoload mechanism to do whatever it needs to do
every time it sees a (define-simple-pcomplete ...) form?

>> +;; What do you think of a macro like this?
>> +(defmacro define-simple-pcomplete (name command &rest args)
>> +  "Create `pcomplete' completions for a simple command.
>> +COMMAND and ARGS are as in `pcomplete-from-help'.  Completion
>> +candidates for this command will include the parsed arguments as
>> +well as files."
>> +  (let* ((namestr (symbol-name name))
>> +         (docstring (if-let ((i (string-search "/" namestr)))
>> +                       (format "Completions for the `%s' command in `%s'."
>> +                               (substring namestr 0 i)
>> +                               (substring namestr i))
>> +                     (format "Completions for the `%s' command." namestr))))
>> +    `(defun ,(intern (concat "pcomplete/" namestr)) ()
>> +       ,docstring
>> +       (pcomplete--simple-command ,command ',args))))
>
>     (defun define-simple-pcomplete (name command &rest args)
>       "Create `pcomplete' completions for a simple command.
>     COMMAND and ARGS are as in `pcomplete-from-help'.  Completion
>     candidates for this command will include the parsed arguments as
>     well as files."
>       (let* ((namestr (symbol-name name))
>              (docstring (if-let ((i (string-search "/" namestr)))
>                            (format "Completions for the `%s' command in `%s'."
>                                    (substring namestr 0 i)
>                                    (substring namestr i))
>                          (format "Completions for the `%s' command." namestr))))
>         (defalias (intern (concat "pcomplete/" namestr))
>           (lambda ()
>            (:documentation docstring)
>            (pcomplete--simple-command command args)))))
>
> Also, we may end up with various "simple" solutions, so I'd use a name
> that is more explicit about how it works.  E.g. `pcomplete-via-help`?
>
>
>         Stefan

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:15:30 +0200

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

On Sat, 10 Sep 2022 at 10:32, Stefan Monnier wrote:

>> I've attached a new iteration of the patch.  I think the git completion
>> should be pretty usable (but certainly can be refined in the future).
>> I'm also satisfied with the parser, please have a look if you are
>> interested.
>
> LGTM, feel free to push it to `master`.

I've attached a new version which I consider good to merge (I don't have
direct access to the repo).  I've also included a tiny fix to Emacs's
help message.

The patch includes completion for a variety of commands (in part to test
the flexibility of the parser), including most commands from coreutils,
so it has become a bit bulky.  Given the large number of new functions,
I hope the commit message is detailed enough.

[0001-src-emacs.c-usage_message-Remove-stray-tabs.patch (text/x-patch, attachment)]

[0002-pcomplete-Generate-completions-from-help-messages.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:21:00 +0200

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> The patch includes completion for a variety of commands (in part to test
> the flexibility of the parser), including most commands from coreutils,
> so it has become a bit bulky.  Given the large number of new functions,
> I hope the commit message is detailed enough.

I've pushed the first (trivial) patch, but the second has two warnings:

In pcomplete/systemctl:
pcmpl-linux.el:154:12: Warning: Unused lexical variable `subcmd'
In pcomplete-from-help:
pcomplete.el:1344:2: Warning: docstring wider than 80 characters

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:41:45 +0200

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

On Wed, 14 Sep 2022 at 21:21, Lars Ingebrigtsen wrote:

> Augusto Stoffel <arstoffel <at> gmail.com> writes:
>
>> The patch includes completion for a variety of commands (in part to test
>> the flexibility of the parser), including most commands from coreutils,
>> so it has become a bit bulky.  Given the large number of new functions,
>> I hope the commit message is detailed enough.
>
> I've pushed the first (trivial) patch, but the second has two warnings:
>
> In pcomplete/systemctl:
> pcmpl-linux.el:154:12: Warning: Unused lexical variable `subcmd'

I've attached a patch to be applied (and squashed) on top of what I sent
previously.  Let me know if this is inconvenient and I'll send the whole
thing.

> In pcomplete-from-help:
> pcomplete.el:1344:2: Warning: docstring wider than 80 characters

Hum, I don't know how to fix this.  The long line is the function
signature, which is created mechanically by cl-defun and displays all
the default values of the keyword arguments.

The formatting is horrible:

    (pcomplete-from-help COMMAND &rest ARGS &key (MARGIN (rx bol (+ " ")))
    (ARGUMENT (rx "-" (+ (any "-" alnum)) (32 "="))) (METAVAR (rx (32 " ")
    (or (+ (any alnum "_-")) (seq "[" (+? nonl) "]") (seq "<" (+? nonl)
    ">") (seq "{" (+? nonl) "}")))) (SEPARATOR (rx ", " symbol-start))
    (DESCRIPTION (rx (* nonl) (* "\n" (>= 9 " ") (* nonl)))) NARROW-START
    NARROW-END)

But the information is good to have, because you need to know what these
regexps are in order to use the function.

[0001-Fix-to-Warning-Unused-lexical-variable-subcmd.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:48:45 +0200

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> I've attached a patch to be applied (and squashed) on top of what I sent
> previously.  Let me know if this is inconvenient and I'll send the whole
> thing.

I'd prefer the whole thing in one patch.


[...]

> +               ("start"
> +                (pcomplete-here
> +                 (pcmpl-linux--systemd-units context "--state" "inactive,failed")))
> +               ((or "restart" "stop")
> +                (pcomplete-here
> +                 (pcmpl-linux--systemd-units context "--state" "active")))

But...  subcmd isn't used here in the new lines, either, so does that
really fix the warning?

> Hum, I don't know how to fix this.  The long line is the function
> signature, which is created mechanically by cl-defun and displays all
> the default values of the keyword arguments.
>
> The formatting is horrible:
>
>     (pcomplete-from-help COMMAND &rest ARGS &key (MARGIN (rx bol (+ " ")))
>     (ARGUMENT (rx "-" (+ (any "-" alnum)) (32 "="))) (METAVAR (rx (32 " ")
>     (or (+ (any alnum "_-")) (seq "[" (+? nonl) "]") (seq "<" (+? nonl)
>     ">") (seq "{" (+? nonl) "}")))) (SEPARATOR (rx ", " symbol-start))
>     (DESCRIPTION (rx (* nonl) (* "\n" (>= 9 " ") (* nonl)))) NARROW-START
>     NARROW-END)
>
> But the information is good to have, because you need to know what these
> regexps are in order to use the function.

Oh, yeah, that's pretty bad...  we should probably fix that in the
cl-defun macro, I guess, so this doesn't have to be fixed in this patch.

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:57:09 +0200

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

On Wed, 14 Sep 2022 at 21:48, Lars Ingebrigtsen wrote:

> Augusto Stoffel <arstoffel <at> gmail.com> writes:
>
>> I've attached a patch to be applied (and squashed) on top of what I sent
>> previously.  Let me know if this is inconvenient and I'll send the whole
>> thing.
>
> I'd prefer the whole thing in one patch.

Voilà.

[0001-pcomplete-Generate-completions-from-help-messages.patch (text/x-patch, attachment)]

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

>
> [...]
>
>> +               ("start"
>> +                (pcomplete-here
>> +                 (pcmpl-linux--systemd-units context "--state" "inactive,failed")))
>> +               ((or "restart" "stop")
>> +                (pcomplete-here
>> +                 (pcmpl-linux--systemd-units context "--state" "active")))
>
> But...  subcmd isn't used here in the new lines, either, so does that
> really fix the warning?

`subcmd' was always there, textually, in the `(pcase subcmd' right above
that.  But in the previous version of the code it disappeared during
macro expansion because it was not compared against anything in the
pcase patterns.

Very neat sanity check, indeed (but I did what I did on purpose because
I wanted to leave the function in an easier shape for future
refinements).

>> Hum, I don't know how to fix this.  The long line is the function
>> signature, which is created mechanically by cl-defun and displays all
>> the default values of the keyword arguments.
>>
>> The formatting is horrible:
>>
>>     (pcomplete-from-help COMMAND &rest ARGS &key (MARGIN (rx bol (+ " ")))
>>     (ARGUMENT (rx "-" (+ (any "-" alnum)) (32 "="))) (METAVAR (rx (32 " ")
>>     (or (+ (any alnum "_-")) (seq "[" (+? nonl) "]") (seq "<" (+? nonl)
>>     ">") (seq "{" (+? nonl) "}")))) (SEPARATOR (rx ", " symbol-start))
>>     (DESCRIPTION (rx (* nonl) (* "\n" (>= 9 " ") (* nonl)))) NARROW-START
>>     NARROW-END)
>>
>> But the information is good to have, because you need to know what these
>> regexps are in order to use the function.
>
> Oh, yeah, that's pretty bad...  we should probably fix that in the
> cl-defun macro, I guess, so this doesn't have to be fixed in this patch.

All right, thanks.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57673 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 21:59:04 +0200

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> `subcmd' was always there, textually, in the `(pcase subcmd' right above
> that.  But in the previous version of the code it disappeared during
> macro expansion because it was not compared against anything in the
> pcase patterns.

Ah, right.

I've now pushed your patch to Emacs 29, and will take a look at fixing
the cl-defun doc string issue tomorrowish.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57673 <at> debbugs.gnu.org, Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 16:40:43 -0400

>>     (pcomplete-from-help COMMAND &rest ARGS &key (MARGIN (rx bol (+ " ")))
>>     (ARGUMENT (rx "-" (+ (any "-" alnum)) (32 "="))) (METAVAR (rx (32 " ")
>>     (or (+ (any alnum "_-")) (seq "[" (+? nonl) "]") (seq "<" (+? nonl)
>>     ">") (seq "{" (+? nonl) "}")))) (SEPARATOR (rx ", " symbol-start))
>>     (DESCRIPTION (rx (* nonl) (* "\n" (>= 9 " ") (* nonl)))) NARROW-START
>>     NARROW-END)
>>
>> But the information is good to have, because you need to know what these
>> regexps are in order to use the function.
>
> Oh, yeah, that's pretty bad...  we should probably fix that in the
> cl-defun macro, I guess, so this doesn't have to be fixed in this patch.

Side note: as a programmer, I don't really want to see this whole mess
in `C-h o` either.

Instead, I'd want the docstring to tell me what is the intended default
value of those keywords (rather than what is the code used to build that
default value).

Tho to be perfectly honest, I think any keyword argument whose default
value is not the same as nil is a problem (I know, sometimes there can
be good reasons for that, but we should try to avoid them as much as
possible).

So rather than

    (cl-defun pcomplete-from-help (command
                                   &rest args
                                   &key
                                   (margin (rx bol (+ " ")))
                                   (argument (rx "-" (+ (any "-" alnum)) (? "=")))
                                   (metavar (rx (? " ")
                                                (or (+ (any alnum "_-"))
                                                    (seq "[" (+? nonl) "]")
                                                    (seq "<" (+? nonl) ">")
                                                    (seq "{" (+? nonl) "}"))))
                                   (separator (rx ", " symbol-start))
                                   (description (rx (* nonl)
                                                    (* "\n" (>= 9 " ") (* nonl))))
                                   narrow-start
                                   narrow-end)

I'd rather see something like:

    (cl-defun pcomplete-from-help (command &rest args
                                   &key margin argument metavar
                                   separator description
                                   narrow-start narrow-end)
      (unless margin (setq margin (rx bol (+ " "))))
      (unless argument (setq argument (rx "-" (+ (any "-" alnum)) (? "=")))
      (unless metavar (setq metavar (rx (? " ")
                                        (or (+ (any alnum "_-"))
                                            (seq "[" (+? nonl) "]")
                                            (seq "<" (+? nonl) ">")
                                            (seq "{" (+? nonl) "}"))))
      (unless separator (setq separator (rx ", " symbol-start))
      (unless description (setq description (rx (* nonl)
                                                (* "\n" (>= 9 " ") (* nonl))))

[ which will probably not fix the overlong line problem, of course.  ]


        Stefan

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 57673 <at> debbugs.gnu.org, Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 23:11:16 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> Tho to be perfectly honest, I think any keyword argument whose default
> value is not the same as nil is a problem (I know, sometimes there can
> be good reasons for that, but we should try to avoid them as much as
> possible).

(I don't agree -- I think non-nil keyword values are fine.)

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

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 23:23:48 +0200

On Wed, 14 Sep 2022 at 16:40, Stefan Monnier wrote:

> Tho to be perfectly honest, I think any keyword argument whose default
> value is not the same as nil is a problem (I know, sometimes there can
> be good reasons for that, but we should try to avoid them as much as
> possible).

Hum, that's a strong claim, and if I heard it from a clean code
influencer I would be rather suspicious.  So I'm curious why you think
that way.

My thinking is that in the body of the function you can do anything at
all, while in the arglist the only thing you can do (sanely) is to
nominate a value.  So if your default _is_ just a plain old value, it's
reassuring to see it declared in the arglist.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 57673 <at> debbugs.gnu.org
Subject: Re: bug#57673: [PATCH] Parse --help messages for pcomplete
Date: Wed, 14 Sep 2022 17:45:34 -0400

>> Tho to be perfectly honest, I think any keyword argument whose default
>> value is not the same as nil is a problem (I know, sometimes there can
>> be good reasons for that, but we should try to avoid them as much as
>> possible).
> Hum, that's a strong claim, and if I heard it from a clean code
> influencer I would be rather suspicious.  So I'm curious why you think
> that way.

It's convenient for the callers to know that they always pass nil to
mean "use the default".

It lets you write:

   (bar baz :hello foo)

instead of

   (if foo
       (bar baz :hello foo)
     (bar baz)

which gets really tiresome when you have more than 1 such keyword arg to
(maybe) pass to the function.

But my opinion is also influenced by the convenience of having only
`put` and `get` and knowing that there's no difference between a nil
property and a property that's absent.  Same holds for alists where it's
really convenient to try and stick to the principle that absent entries
are equivalent to entries associated to nil.

The convenience comes mostly from the fact that it's pervasive, rather
than from any specific use case.


        Stefan

Previous Next

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