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
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4