From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Mathias Dahl Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Add abbrev suggestions Date: Wed, 12 Aug 2020 00:16:33 +0200 Message-ID: References: <83pn8rgwib.fsf@gnu.org> <83o8o4c8s4.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000003b898a05aca16de9" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4781"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Aug 12 00:17:26 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 1k5caE-00016W-0I for ged-emacs-devel@m.gmane-mx.org; Wed, 12 Aug 2020 00:17:26 +0200 Original-Received: from localhost ([::1]:52310 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1k5caC-0004iq-Vs for ged-emacs-devel@m.gmane-mx.org; Tue, 11 Aug 2020 18:17:25 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:35694) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k5cZe-0004IB-TF for emacs-devel@gnu.org; Tue, 11 Aug 2020 18:16:50 -0400 Original-Received: from mail-vs1-xe43.google.com ([2607:f8b0:4864:20::e43]:35074) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1k5cZc-0000wx-I1; Tue, 11 Aug 2020 18:16:50 -0400 Original-Received: by mail-vs1-xe43.google.com with SMTP id j188so114294vsd.2; Tue, 11 Aug 2020 15:16:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gLNRQ6Q+VwYzwplHsGZ0JX3ulHGJ4wXadgi6Mdd3PHo=; b=SgHY7mgSHHACx3B7Q+C1+HdJUoISN1o42w9ReQMbZR6fRG+dLUBVLGosXSCC7sc1Jy Kev0c9/XGMsFSElvC0t3WCF3UJCMwIQxGktOcpTRr8m6FPGwofM1Xbwll513aSdPir6k pPFGQ0PWHHcud9YQF+KwxqfVOK14BNin/ubXWjg3Zw1ehuhEC5anDbgAYGgmISLP34G5 jZddB1YSg5ad8ilgEqjmceKKMbYnV/6Eyp1fbGj02YIejlNCvC55Z1Ap8Uik5MraYrBk XGs12Dg3a+COhKZUc1X3Xbkd7s2lb3EbmjDwCLCkCUJOmMfub+HKNk+hQpZSO2gbMhAD x4yA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gLNRQ6Q+VwYzwplHsGZ0JX3ulHGJ4wXadgi6Mdd3PHo=; b=JqM1YXzavVtsc8BFMCNUeQxGtJoZJ7SqwBYjsnXbGwJeY7M1fSGlTLswdp5DrWGe5N 4dMhM+PnQFMoe8yPSBkstHIq36bY7cO+twBQhA9EBLfT2P8DfejwQLypoDQm89efd/rC bW340evXUG6/vKYgB7rCiEQKAcvQLcE7V1llxVp2sRt/gfbUuaUmnzAmg4qUTocq8zVK kkcBA5kCszdD0yWDAmobGzuIop0vOKJ51MQ2IPcSXx4OqJ2cvMDhEQp/8Hwa8xiwzHIq V3YS3KTocNjqSs8AtToWV8GCU9Mohe0VLE7c+QAqZGyOLxHnp7Y7ai0U8ZSF8FBus9lu OnlA== X-Gm-Message-State: AOAM533dW3Lp3nmFck/4W9Sr7wBeABv2fmLCp0MbF3Ti0jQwXKpdvfdN kry1/cEXxswJ5Uum03FNRz07RZZab3ZcN4ggM4wFVJD6fHs= X-Google-Smtp-Source: ABdhPJyYzBZd2DGiZzUZGfJr/CZX46duptLsOGi4BrACAFk5WLhBF6wW/V6JO/ORkT7bXkSgrjnfbwjMA5V+vsu7avc= X-Received: by 2002:a67:3354:: with SMTP id z81mr24864851vsz.39.1597184205882; Tue, 11 Aug 2020 15:16:45 -0700 (PDT) In-Reply-To: <83o8o4c8s4.fsf@gnu.org> Received-SPF: pass client-ip=2607:f8b0:4864:20::e43; envelope-from=mathias.dahl@gmail.com; helo=mail-vs1-xe43.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. 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, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, 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.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:253648 Archived-At: --0000000000003b898a05aca16de9 Content-Type: text/plain; charset="UTF-8" Hi Eli, > I have a few comments to your patch, mostly about the documentation > parts: Thanks for taking the time to review this. > First, please accompany the patch with a ChangeLog-style commit log > message which describes the individual changes. See CONTRIBUTE for > the style guidance. Will do. > > (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? No idea :), will fix it. > > +(defcustom abbrev-suggest t > > + "Non-nil means we should suggest abbrevs to the user. > > Our style is "Non-nil means suggest abbrevs ..." Got it! > Please always specify a :version tag in new and modified defcustoms > (here and elsewhere in your patch). Sure. > Are you sure it is a good idea to make this non-nil by default? > Wouldn't some users consider these suggestions an annoyance? No, I'm not. In fact, it was not the default when I started working on this, but Stefan suggested that it might be a good default. Now we're me and you against him, I guess... :) > > +(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. I trust you. > > +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. I'll rework it. > > +(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. Yes. > > +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. I know, I was not happy when I wrote that. "we", here, is something like "the difference in length between what the user typed and the abbrev that we found." I guess I could not find a good way to keep the first sentence of the docstring short, so I opted for the fuzzy "we" expression... > > +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. I know. Frankly I don't know if I can come up with a suggestion that combines that together with having a relatively short first sentence... > > +(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. Should be easy to fix. > > +(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. I'll fix it. > > +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. Of course :) > Finally, I think these new features should be in NEWS and probably > also in the user manual. Agree. Should I include those changes in the same patch and resend that when done? > Thank you for working on this. Welcome. I think this would be a useful addition, for me and others, and that makes it fun. Thank you for doing what you do best, and keep on top of all the Emacs things :) /Mathias On Sat, Jul 25, 2020 at 10:13 AM Eli Zaretskii wrote: > > 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. > --0000000000003b898a05aca16de9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Eli,

> I have a few comments to your patch, m= ostly about the documentation
> parts:

Thanks for taking the t= ime to review this.

> First, please accompany the patch with a Ch= angeLog-style commit log
> message which describes the individual cha= nges.=C2=A0 See CONTRIBUTE for
> the style guidance.

Will do.<= br>
> > =C2=A0(defvar abbrev-expand-function #'abbrev--default= -expand
> > - =C2=A0"Function that `expand-abbrev' uses t= o perform abbrev expansion.
> > + =C2=A0 =C2=A0"Function that= `expand-abbrev' uses to perform abbrev expansion.
>
> Wha= t is this whitespace change about?

No idea :), will fix it.

&= gt; > +(defcustom abbrev-suggest t
> > + =C2=A0 =C2=A0"Non= -nil means we should suggest abbrevs to the user.
>
> Our styl= e is "Non-nil means suggest abbrevs ..."

Got it!

&g= t; Please always specify a :version tag in new and modified defcustoms
&= gt; (here and elsewhere in your patch).

Sure.

> Are you su= re it is a good idea to make this non-nil by default?
> Wouldn't = some users consider these suggestions an annoyance?

No, I'm not.= In fact, it was not the default when I started working on
this, but Ste= fan suggested that it might be a good default. Now we're
me and you = against him, I guess... :)

> > +(defcustom abbrev-suggest-hint= -threshold 3
> > + =C2=A0 =C2=A0"Threshold for when to inform= the user that there is an abbrev.
> > +The threshold is the numbe= r of characters that differs between
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ^= ^^^^^^
> "differ", in plural.=C2=A0 I think.

I trust= you.

> > +the length of the abbrev and the length of the expa= nsion.=C2=A0 The
> > +thinking is that if the expansion is only on= e or a few characters
> > +longer than the abbrev, the benefit of = informing the user is not
> > +that big.=C2=A0 If you always want = to be informed, set this value to
> > +`0' or less."
&= gt;
> This doc string should mention abbrev-suggest, since it only h= as
> effect if that is non-nil.

I'll rework it.

>= ; > +(defun abbrev--suggest-get-previous-words (n)
> > + =C2=A0= =C2=A0"Return the previous N words, spaces included.
>
>= "Previous" as in "before point", I presume?=C2=A0 If s= o, please say that.

Yes.

> > +Changes newlines into spa= ces."
> > + =C2=A0 =C2=A0(let ((end (point)))
> > + = =C2=A0 =C2=A0 =C2=A0(save-excursion
> > + =C2=A0 =C2=A0 (backward-= word n)
> > + =C2=A0 =C2=A0 (replace-regexp-in-string
> >= + =C2=A0 =C2=A0 =C2=A0"\\s " " "
> > + =C2=A0= =C2=A0 =C2=A0(buffer-substring-no-properties (point) end)))))
> >= +
> > +(defun abbrev--suggest-above-threshold (expansion)
>= > + =C2=A0 =C2=A0"Return t if we are above the threshold.
> =
> Who is "we" in this context?=C2=A0 This should be explai= ned.

I know, I was not happy when I wrote that. "we", here= , is something
like "the difference in length between what the user= typed and the
abbrev that we found." I guess I could not find a go= od way to keep the
first sentence of the docstring short, so I opted for= the fuzzy "we"
expression...

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

I know. Frankly I don'= ;t know if I can come up with a suggestion that
combines that together w= ith having a relatively short first
sentence...

> > +(defun= abbrev--suggest-shortest-abbrev (new current)
> > + =C2=A0 =C2=A0= "Return the shortest abbrev.
> > +NEW and CURRENT are cons ce= lls where the `car' is the expansion
>
> Same here.
Should be easy to fix.

> > +(defun abbrev--suggest-get-totals= ()
> > + =C2=A0 =C2=A0"Return a list of all expansions and t= heir usage.
> > +Each expansion is a cons cell where the `car'= is the expansion
> > +and the `cdr' is the number of times th= e expansion has been
> > +typed."
>
> This doc s= tring doesn't explain the meaning of "usage" in this case.
I'll fix it.

> > +Below is a list of expansions for w= hich abbrevs are defined, and
> > +the number of times the expansi= on was typed manually. To display
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0^^
> Two spaces between sentences, please.

Of= course :)

> Finally, I think these new features should be in NEW= S and probably
> also in the user manual.

Agree. Should I incl= ude those changes in the same patch and resend
that when done?

&g= t; Thank you for working on this.

Welcome. I think this would be a u= seful addition, for me and others,
and that makes it fun.

Thank y= ou for doing what you do best, and keep on top of all the Emacs
things := )

/Mathias


On Sat, Jul 25, 2020 at 10:13 AM Eli Zaretskii <= ;eliz@gnu.org> wrote:
> Date: Sun, 19 Jul 2020 22= :01:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-dev= el@gnu.org
>
> > From: Mathias Dahl <mathias.dahl@gmail.com>
> > Date: Sun, 19 Jul 2020 19:40:17 +0200
> >
> > Could someone help out with the below?
>
> Sorry your patch was forgotten.=C2=A0 If no one beats me to it, I will= take
> care of it in a few days.=C2=A0 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.=C2=A0 See CONTRIBUTE for the style guidance.

>=C2=A0 (defvar abbrev-expand-function #'abbrev--default-expand
> -=C2=A0 "Function that `expand-abbrev' uses to perform abbrev= expansion.
> +=C2=A0 =C2=A0 "Function that `expand-abbrev' uses to perform= abbrev expansion.

What is this whitespace change about?

> +(defcustom abbrev-suggest t
> +=C2=A0 =C2=A0 "Non-nil means we should suggest abbrevs to the us= er.

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."
> +=C2=A0 =C2=A0 :type 'boolean
> +=C2=A0 =C2=A0 :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
> +=C2=A0 =C2=A0 "Threshold for when to inform the user that there = is an abbrev.
> +The threshold is the number of characters that differs between
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 ^^^^^^^
"differ", in plural.=C2=A0 I think.

> +the length of the abbrev and the length of the expansion.=C2=A0 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.=C2=A0 If you always want to be informed, set this value to<= br> > +`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)
> +=C2=A0 =C2=A0 "Return the previous N words, spaces included.

"Previous" as in "before point", I presume?=C2=A0 If so= , please say that.

> +Changes newlines into spaces."
> +=C2=A0 =C2=A0 (let ((end (point)))
> +=C2=A0 =C2=A0 =C2=A0 (save-excursion
> +=C2=A0 =C2=A0 =C2=A0(backward-word n)
> +=C2=A0 =C2=A0 =C2=A0(replace-regexp-in-string
> +=C2=A0 =C2=A0 =C2=A0 "\\s " " "
> +=C2=A0 =C2=A0 =C2=A0 (buffer-substring-no-properties (point) end)))))=
> +
> +(defun abbrev--suggest-above-threshold (expansion)
> +=C2=A0 =C2=A0 "Return t if we are above the threshold.

Who is "we" in this context?=C2=A0 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)
> +=C2=A0 =C2=A0 "Return the shortest abbrev.
> +NEW and CURRENT are cons cells where the `car' is the expansion
Same here.

> +(defun abbrev--suggest-get-totals ()
> +=C2=A0 =C2=A0 "Return a list of all expansions and their usage.<= br> > +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 thi= s case.

> +Below is a list of expansions for which abbrevs are defined, and
> +the number of times the expansion was typed manually. To display
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^
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.
--0000000000003b898a05aca16de9--