[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#67275: [PATCH] ; Improve and add tests for Completion Preview mode
From: |
Eshel Yaron |
Subject: |
bug#67275: [PATCH] ; Improve and add tests for Completion Preview mode |
Date: |
Sun, 19 Nov 2023 12:23:16 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
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:
v2-0001-Improve-and-add-tests-for-Completion-Preview-mode.patch
Description: Text Data