Re: [PATCH] `affixation-function`: Allow only three-element lists

[Daniel Mendler]

On 4/25/21 7:46 PM, Juri Linkov wrote:
> > @@ -2110,7 +2110,9 @@ minibuffer-completion-help
> >                         (cond
> >                          (aff-fun
> >                           (setq completions
> > -                              (funcall aff-fun completions)))
> > +                              (funcall aff-fun completions))
> > +                        (cl-assert (or (not completions)
> > +                                       (= 3 (length (car completions))))))
>
> It's surprising to see that you added such artificial restriction.
> I expected a change with something more like this:
>
> and then simplify the implementation of completion--insert-strings
> by removing all complexity that handled suffix-only completion strings.

Yes, this is certainly a less artificial way to restrict the results of 
the `affixation-function`. However this works only if you add the 
condition on when to add annotation faces as you proposed below.

> > diff --git a/lisp/simple.el b/lisp/simple.el
> > index 999755a642..26eb8cad7f 100644
> > --- a/lisp/simple.el
> > +++ b/lisp/simple.el
> > @@ -2080,8 +2080,11 @@ read-extended-command--affixation
> >                               (obsolete
> >                                (format " (%s)" (car obsolete)))
> >                               ((and binding (not (stringp binding)))
> > -                             (format " (%s)" (key-description binding))))))
> > -         (if suffix (list command-name suffix) command-name)))
> > +                             (format " (%s)" (key-description binding)))
> > +                            (t ""))))
> > +         (put-text-property 0 (length suffix)
> > +                            'face 'completions-annotations suffix)
> > +         (list command-name "" suffix)))
> >        command-names)))
>
> Why such change only for one particular use of annotations?
> Shouldn't completion--insert-strings check if the suffix is an
> empty string, and then put the 'completions-annotations' face on it?

I did not do this since I wanted to avoid magic which complicates the 
definition of the `affixation-function`. The idea was to never add faces 
to the annotations supplied by the `affixation-function`.

I am looking at the `affixation-function` from the perspective of how 
the definition can be made as simple/predicatable as possible for the 
API user since there are other completion UIs and many completion tables 
which will make use of this. I treat the completion UI behind the API as 
a blackbox.

In contrast you take the perspective of how the affixation function 
could be fitted best into the existing completion code without 
introducing artificial restrictions.

I argue that it is better to take a step back from the actual code base 
in minibuffer.el and adopt the simpler definition.

Daniel