Hello Filipp,

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> I've only skimmed through previous discussion, but will point out a few
> things in the patch.

Thanks for your helpful review! Updated patch at the very bottom of this
message.

> [...]
>> +;;; Usage:
>> +
>> +;;    (require 'eudc-capf)
>> +;;    (add-hook 'completion-at-point-functions #'eudc-capf-complete -1 t)
>
> As you're showing add-hook with LOCAL t, perhaps you should also show
> some context, to avoid users pasting this directly into their .emacs.

Good point. I've added a few more words of comment.

>> +(defconst eudc-capf-modes '(message-mode) "List of modes in which email \
>> +address completion is to be attempted.")
>
> Put docstring on its own line.

Done.

>> +  (if (and (seq-some #'derived-mode-p eudc-capf-modes)
>> +           (let ((mail-abbrev-mode-regexp message-email-recipient-header-regexp))
>> +             (mail-abbrev-in-expansion-header-p)))
>> +      (eudc-capf-message-expand-name)
>
> You don't need to specify else-branch for an if.  Just omit it.  (same
> below)

Ah, well spotted. Removed.

>> +`completion-at-point'."
>> +  (if (or (and (boundp 'eudc-server) eudc-server)
>> +          (and (boundp 'eudc-server-hotlist) eudc-server-hotlist))
>
> You're requiring eudc, why should these be unbound?

Another well spotted one. I borrowed this fragment from some other code
I wrote, where eudc is not being required. Boundp removed.

>> +      (progn
>> +        (setq-local completion-styles '(substring partial-completion))
>> +        (let* ((beg (save-excursion
>> +                      (if (re-search-backward "\\([:,]\\|^\\)[ \t]*"
>> +                                              (point-at-bol) 'move)
>
> t instead of 'move?

Hm. Quoting from the docstring (re-search-forward in this case):
---------------------------- Begin Quote -----------------------------
The optional third argument NOERROR indicates how errors are handled
  when the search fails. If it is nil or omitted, emit an error; if it
  is t, simply return nil and do nothing; if it is neither nil nor t,
  move to the limit of search and return nil.
----------------------------- End Quote ------------------------------

This reads as if t ("do nothing") vs. 'move ("move to the limit of
search") should make a difference? A few quick experiments seem to
indicate that in practice both seem to behave the same though. In this
light, I'm fine with changing 'move to t.

> I also don't like the side-effect of setting completion-styles for the
> user, although only locally. 

Me neither.

> Can it be done in some other way?

Short answer: I don't think so.

Longer answer:

The motivation for putting this is that completion-at-point filters the
completion tables based on completion-styles. Thus, if completion-styles
is set too restrictive, useful results may not be offered to the user.
The default completion-styles in message-mode is '(basic
partial-completion emacs22). I wanted 'substring to be the primary as it
matches the search term anywhere within the candidate, as opposed to
'basic which matches it at the beginning only. In this respect, you
might view 'substring as a super-set of 'basic.

That said, it would probably be desirable for message mode to have
different values for both, completion-at-point-functions and
completion-styles, depending on where point is (email header, newsgroup
header, message body, etc.). But this seems like a wider discussion
about the architecture of message.el rather than this patch.


Many thanks and looking forward to your thoughts,

  --alexander