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: