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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 19 Nov 2023 11:25:55 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +(defun completion-preview--try-table (table beg end props)
> +  "Check TABLE for a completion matching the text between BEG and END.
> +
> +PROPS is a property list with additional information about TABLE.
> +See `completion-at-point-functions' for more details.
> +
> +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.

> +(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.

> +`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.

Thanks.