From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Add abbrev suggestions Date: Sat, 25 Jul 2020 11:12:59 +0300 Message-ID: <83o8o4c8s4.fsf@gnu.org> References: <83pn8rgwib.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="16848"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: mathias.dahl@gmail.com Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Jul 25 10:13:52 2020 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 1jzFJX-0004I3-D8 for ged-emacs-devel@m.gmane-mx.org; Sat, 25 Jul 2020 10:13:51 +0200 Original-Received: from localhost ([::1]:39302 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jzFJW-0006Hy-Fy for ged-emacs-devel@m.gmane-mx.org; Sat, 25 Jul 2020 04:13:50 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:45372) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jzFIv-0005rX-Sn for emacs-devel@gnu.org; Sat, 25 Jul 2020 04:13:13 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:41967) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jzFIv-0002cX-K0; Sat, 25 Jul 2020 04:13:13 -0400 Original-Received: from [176.228.60.248] (port=2323 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jzFIu-0004sW-TR; Sat, 25 Jul 2020 04:13:13 -0400 In-Reply-To: <83pn8rgwib.fsf@gnu.org> (message from Eli Zaretskii on Sun, 19 Jul 2020 22:01:00 +0300) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 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" Xref: news.gmane.io gmane.emacs.devel:253207 Archived-At: > Date: Sun, 19 Jul 2020 22:01:00 +0300 > From: Eli Zaretskii > Cc: emacs-devel@gnu.org > > > From: Mathias Dahl > > Date: Sun, 19 Jul 2020 19:40:17 +0200 > > > > Could someone help out with the below? > > Sorry your patch was forgotten. If no one beats me to it, I will take > care of it in a few days. Stay tuned. I have a few comments to your patch, mostly about the documentation parts: First, please accompany the patch with a ChangeLog-style commit log message which describes the individual changes. See CONTRIBUTE for the style guidance. > (defvar abbrev-expand-function #'abbrev--default-expand > - "Function that `expand-abbrev' uses to perform abbrev expansion. > + "Function that `expand-abbrev' uses to perform abbrev expansion. What is this whitespace change about? > +(defcustom abbrev-suggest t > + "Non-nil means we should suggest abbrevs to the user. Our style is "Non-nil means suggest abbrevs ..." > +By enabling this option, if abbrev mode is enabled and if the > +user has typed some text that exists as an abbrev, suggest to the > +user to use the abbrev instead." > + :type 'boolean > + :group 'abbrev-mode) Please always specify a :version tag in new and modified defcustoms (here and elsewhere in your patch). Are you sure it is a good idea to make this non-nil by default? Wouldn't some users consider these suggestions an annoyance? > +(defcustom abbrev-suggest-hint-threshold 3 > + "Threshold for when to inform the user that there is an abbrev. > +The threshold is the number of characters that differs between ^^^^^^^ "differ", in plural. I think. > +the length of the abbrev and the length of the expansion. The > +thinking is that if the expansion is only one or a few characters > +longer than the abbrev, the benefit of informing the user is not > +that big. If you always want to be informed, set this value to > +`0' or less." This doc string should mention abbrev-suggest, since it only has effect if that is non-nil. > +(defun abbrev--suggest-get-previous-words (n) > + "Return the previous N words, spaces included. "Previous" as in "before point", I presume? If so, please say that. > +Changes newlines into spaces." > + (let ((end (point))) > + (save-excursion > + (backward-word n) > + (replace-regexp-in-string > + "\\s " " " > + (buffer-substring-no-properties (point) end))))) > + > +(defun abbrev--suggest-above-threshold (expansion) > + "Return t if we are above the threshold. Who is "we" in this context? This should be explained. > +EXPANSION is a cons cell where the car is the expansion and the > +cdr is the abbrev." Our style is to include the arguments in the first sentence of the doc string. > +(defun abbrev--suggest-shortest-abbrev (new current) > + "Return the shortest abbrev. > +NEW and CURRENT are cons cells where the `car' is the expansion Same here. > +(defun abbrev--suggest-get-totals () > + "Return a list of all expansions and their usage. > +Each expansion is a cons cell where the `car' is the expansion > +and the `cdr' is the number of times the expansion has been > +typed." This doc string doesn't explain the meaning of "usage" in this case. > +Below is a list of expansions for which abbrevs are defined, and > +the number of times the expansion was typed manually. To display ^^ Two spaces between sentences, please. Finally, I think these new features should be in NEWS and probably also in the user manual. Thank you for working on this.