bug#67275: [PATCH] ; Improve and add tests for Completion Preview mode

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 67275@debbugs.gnu.org
> Date: Sun, 19 Nov 2023 12:23:16 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> +When TABLE contains a matching completion, return a list
> >> +\(PREVIEW BEG END ALL EXIT-FN) where PREVIEW is the text to show
> >> +in the completion preview, ALL is the list of all matching
> >> +completion candidates, and EXIT-FN is either a function to call
> >> +after inserting PREVIEW or nil.  When TABLE does not contain
> >> +matching completions, or when there are multiple matching
> >> +completions and `completion-preview-exact-match-only' is non-nil,
> >> +return nil instead."
> >
> > It is better to use "if" here where you use "when".  "When" can be
> > interpreted as a time-related condition, which is not what you want
> > here.
> 
> Right, done in the updated patch (v2) below.
> 
> >> +(defun completion-preview--capf-wrapper (capf)
> >> +  "Translate output of CAPF to properties for completion preview overlay.
> >> +
> >> +If CAPF returns a list (BEG END TABLE . PROPS), call
> >
> > If CAPF _returns_ something, it is probably a function.  But then why
> > does the first sentence say "output of CAPF"? functions in ELisp don't
> > "output" stuff.
> 
> Thanks, I've replaced "output" with "return value".
> 
> >> +`completion-preview--try-table' to check TABLE for matching
> >> +completion candidates.  If `completion-preview--try-table'
> >> +returns a non-nil value, return that value.  Otherwise, return a
> >> +list with nil car which means that completion failed, unless
> >> +PROPS includes the property `:exclusive' with value `no', in
> >> +which case this function returns nil which means to try other
> >> +functions from `completion-at-point-functions'."
> >
> > This basically tells in words what the code does.  But since the code
> > is quite simple, I wonder why we need this in the doc string.  The
> > disadvantage of having this in the doc string is that we'd need to
> > update it each time the code changes.
> >
> > Instead, think if something in what the code does needs to be
> > explained _beyond_ what the code itself says, like if you need to
> > explain the reasons why the code does what it does, or why you access
> > this or that property, and explain that -- in comments, not in the doc
> > string.  The doc string should ideally be a higher-level description
> > of the function's purpose and the meaning of its return values.
> 
> Makes sense, thanks.  I removed the lengthy description and added a
> comment explaining the only non-obvious part.
> 
> 
> Here's the updated patch:

Thanks, now installed on master.