From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Thomas Fitzsimmons Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] new EUDC backends for ecomplete, and mailabbrev (with ERT tests) Date: Tue, 08 Nov 2022 21:27:30 -0500 Message-ID: References: <8e9c60585677321e437a29215963a908@condition-alpha.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4399"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) Cc: Alexander Adolf , emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Nov 09 03:28:25 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1osapE-0000yj-9G for ged-emacs-devel@m.gmane-mx.org; Wed, 09 Nov 2022 03:28:25 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1osaoT-0003Hp-RO; Tue, 08 Nov 2022 21:27:37 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1osaoS-0003Hd-5w for emacs-devel@gnu.org; Tue, 08 Nov 2022 21:27:36 -0500 Original-Received: from mail.fitzsim.org ([69.165.165.189]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1osaoP-0003TV-Pn for emacs-devel@gnu.org; Tue, 08 Nov 2022 21:27:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=fitzsim.org ; s=20220430; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=x3azopZuKZkYjSf38GBXVm+INMTGLuqwE0b7z1aPkio=; b=PJ+fM5kL5vuSbPoiwOtWRUJFZL /E2SHJmE+m6ulFiH7xxY7rLOSlx/8BInFhFw++xN5mQzF+GHA7KeTPypJY/Ebp9AOgWNTNMToPAcf k8hMLtzw8kVnCJ5HvXytJSfxzUxCab0hQiOPIPVHyfA9/guL5EjwIOKtSmTJBcu/C6Ej2xmjHUCqD 8JBDjmDddRUI83wkFMrb9IgoPwT9FKQSB+0f/IqUd53jh8ZKcJ7mOf/0EVPcN9Br7vM16qWvVcp0O 1TI+/Tp78iFTwBiMD9kZ0lckSRWNsmgCOHcfVP6HYbHFjR+HyL5pcZUMiFY1N6IM4BDKae9qFVQc3 mrtWbmug==; Original-Received: from [192.168.1.1] (helo=localhost.localdomain) by mail.fitzsim.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1osaoM-0007Zi-Tg; Tue, 08 Nov 2022 21:27:31 -0500 In-Reply-To: (Stefan Monnier's message of "Tue, 16 Aug 2022 21:47:19 -0400") Received-SPF: pass client-ip=69.165.165.189; envelope-from=fitzsim@fitzsim.org; helo=mail.fitzsim.org X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:299376 Archived-At: Hi Stefan, I addressed your concerns and pushed this patch. I'll comment on each suggestion inline. Stefan Monnier 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