From: Mathias Dahl <mathias.dahl@gmail.com>
To: Eli Zaretskii <eliz@gnu.org>
Cc: emacs-devel@gnu.org
Subject: Re: [PATCH] Add abbrev suggestions
Date: Wed, 12 Aug 2020 00:16:33 +0200 [thread overview]
Message-ID: <CABrcCQ4O+OpS9mPmxUkJgWuLPCXax7ZRvyRwmDeJThqs2dNQYA@mail.gmail.com> (raw)
In-Reply-To: <83o8o4c8s4.fsf@gnu.org>
[-- Attachment #1: Type: text/plain, Size: 8192 bytes --]
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 <eliz@gnu.org> wrote:
> > Date: Sun, 19 Jul 2020 22:01:00 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: emacs-devel@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. 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.
>
[-- Attachment #2: Type: text/html, Size: 10496 bytes --]
next prev parent reply other threads:[~2020-08-11 22:16 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-05 23:40 [PATCH] Add abbrev suggestions Mathias Dahl
2020-07-19 17:40 ` Mathias Dahl
2020-07-19 19:01 ` Eli Zaretskii
2020-07-25 8:12 ` Eli Zaretskii
2020-08-11 22:16 ` Mathias Dahl [this message]
2020-08-13 13:59 ` Eli Zaretskii
2020-08-13 14:29 ` Mathias Dahl
2020-09-14 22:04 ` Mathias Dahl
2020-09-15 6:20 ` Andreas Röhler
2020-09-18 8:39 ` Mathias Dahl
2020-09-15 8:16 ` Robert Pluim
2020-09-18 8:40 ` Mathias Dahl
2020-09-24 20:02 ` Mathias Dahl
2020-09-25 8:09 ` Robert Pluim
2020-09-25 20:42 ` Mathias Dahl
2020-09-26 14:19 ` Robert Pluim
2020-09-26 20:56 ` Mathias Dahl
2020-09-26 22:21 ` Stefan Monnier
2020-09-27 6:12 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABrcCQ4O+OpS9mPmxUkJgWuLPCXax7ZRvyRwmDeJThqs2dNQYA@mail.gmail.com \
--to=mathias.dahl@gmail.com \
--cc=eliz@gnu.org \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).