Re: [PATCH] Add abbrev suggestions

[Mathias Dahl]

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.