Re: [PATCH] new EUDC backends for ecomplete, and mailabbrev (with ERT tests)

[Thomas Fitzsimmons]

Hi Stefan,

I addressed your concerns and pushed this patch.  I'll comment on each
suggestion inline.

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Alexander Adolf [2022-08-17 02:11:21] wrote:
>> +To enable the ecomplete backend, first `require' the respective
>> +library to load it, and then set the `eudc-server' to localhost in
>> +your init file:
>
> [ I think those `...' should be replace with @code{...} or something
>   like that.  ]

Done, and fixed other occurrences in the file.

> I think we should be able to make the setup more intuitive.  The most
> immediate problem I see is that there's nothing in "set the
> `eudc-server' to localhost" which refers to ecomplete, so as a reader
> I'm left wondering why that would help.
>
>> +@lisp
>> +(require 'eudcb-ecomplete)
>> +(eudc-ecomplete-set-server "localhost")
>> +@end lisp
>
> It's better to avoid using `require` inside the init file:
> - As a general rule, loading an ELisp file should not significantly affect
>   Emacs's behavior.
> - `require` has to load the file right away, slowing down startup.
> The second point is moot here, admittedly, since calling
> `eudc-ecomplete-set-server` will need eudc-ecomplete anyway (either via
> `require` or via autoloading), but still it's better to autoload that
> function so the users don't need to `require` the library explicitly.
>
> Even better would be to replace the above with something like:
>
>     (setq eudb-foo-bar 'ecomplete)
>
> so the EUDB files don't need to be loaded at startup.
>
> [ FWIW, I also happen to believe that Ecomplete should generally be
>   enabled by default, so there should be *no* configuration needed at
>   all for it to work.  ]

I got rid of the server setting functions, and instead defaulted
eudc-server-hotlist to the built-in backends, and I autoloaded the
necessary functions.  Now there is no need for anything in the
initialization file.

Can you try the eudcb-ecomplete.el Usage steps, and see if they work as
you'd expect:

    C-x m lars C-u M-x eudc-expand-try-all RET

>> +;;; Commentary:
>> +;;    This library provides an interface to the ecomplete package as
>> +;;    an EUDC data source.
>> +
>> +;;; Usage:
>> +;;    To load the library, first `require' it:
>> +;;
>> +;;      (require 'eudcb-ecomplete)
>
> See above.

Gone.

>> +;;    In the simplest case then just use:
>> +;;
>> +;;      (eudc-ecomplete-set-server "localhost")
>
> Why have a dummy argument?

Gone.

>> +(defvar eudc-ecomplete-attributes-translation-alist
>> +  '((email     . mail))
>> +  "See `eudc-protocol-attributes-translation-alist'.
>> +The back-end-specific attribute names are used as the \"type\" of
>> +entry when searching, and they must hence match the types you use
>> +in your ecmompleterc database file.")
>            ^^^^^^^^^^^^
>            typo

Fixed.

>> +;; hook ourselves into the EUDC framework
>> +(eudc-protocol-set 'eudc-query-function
>> +               'eudc-ecomplete-query-internal
>> +               'ecomplete)
>> +(eudc-protocol-set 'eudc-list-attributes-function
>> +               nil
>> +               'ecomplete)
>
> [ Sounds like EUDC could make use of `cl-generic` :-)  ]

Yes, probably, but I'm leaving it as-is for now.

>> +(defun eudc-ecomplete-query-internal (query &optional _return-attrs)
>> +  "Query `ecomplete' with QUERY.
>> +QUERY is a list of cons cells (ATTR . VALUE).  Since `ecomplete'
>> +does not provide attributes in the usual sense, the
>> +back-end-specific attribute names in
>> +`eudc-ecomplete-attributes-translation-alist' are used as the
>> +KEY (that is, the \"type\" of match) when looking for matches in
>> +`ecomplete-database'.
>> +
>> +RETURN-ATTRS is a list of attributes to return, defaulting to
>> +`eudc-default-return-attributes'."
>> +  (ecomplete-setup)
>> +  (let ((email-attr (car (eudc-translate-attribute-list '(email))))
>
> Why do we ignore `return-attrs` and hardcode `email` instead (and make
> the docstring lie in the process)?

Alexander and I may have discussed this, but I don't remember the
details now.  I left a FIXME in the source code.  Alexander, can you
send a patch that fixes it or adds a comment answering Stefan's
question?

>> +            ;; special case email: try to decompose
>
> As a convention we like treat our comments with respect; granting them
> the same capitalization and punctuation we'd use in normal text.

Fixed.

>> +(eudc-register-protocol 'ecomplete)
>
> I think it would be good to have `ecomplete` registered as a supported
> protocol before this file is loaded.

I'm not sure what you mean here.  The ecomplete backend follows the same
registration approach as all the other EUDC backends.  I did add the new
backends to "eudc-known-protocols".

> Oh, and thanks for the tests.
> I haven't looked at the mailabbrev patch, sorry.

I tested the mailabbrev usage, and it worked for me with starting from
"emacs -Q", then doing the Usage steps mentioned in eudcb-mailabbrev.el.

Thomas