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

[Stefan Monnier <monnier@iro.umontreal.ca>]

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.  ]

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.  ]

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

> +;;    In the simplest case then just use:
> +;;
> +;;      (eudc-ecomplete-set-server "localhost")

Why have a dummy argument?

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

> +;; 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` :-)  ]

> +(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)?

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

> +(eudc-register-protocol 'ecomplete)

I think it would be good to have `ecomplete` registered as a supported
protocol before this file is loaded.

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


        Stefan