Re: [PATCH] `completing-read`: Add `group-function` support to completion metadata (REVISED PATCH VERSION 2)

[Daniel Mendler]

On 5/1/21 9:54 PM, Juri Linkov wrote:
> I think both completions-detailed and completions-group should be
> checked only by the API user like in help-fns.el.  Otherwise,
> there is duplication that you can see in my previous patch
> for read-char-by-name:
> 
>   (let* ((enable-recursive-minibuffers t)
>        (completion-ignore-case t)
>        (completion-tab-width 4)
>        (completions-group read-char-by-name-group)
>                             =======================
>        (input
>         (completing-read
>          prompt
>          (lambda (string pred action)
>            (if (eq action 'metadata)
>                `(metadata
>                  (group-function
>                   . ,(when read-char-by-name-group
>                              =======================
>                        #'mule--ucs-names-group))
> 
> The same user option read-char-by-name-group is checked twice.
> It should suffice to leave only the latter.

This is a matter of preference. In this case I think I would prefer to
have the settings checked only once centrally in order to avoid the code
duplicatication in every completion table. Furthermore it seems that the
style to check the setting locally in every completion table will lead
to an unnecessary proliferation of configuration variables, since you
introduced the variable `read-char-by-name-group` here. I don't think we
should introduce an extra configuration variable per completion table.

Daniel