[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add abbrev suggestions
Re: [PATCH] Add abbrev suggestions
Sat, 25 Jul 2020 11:12:59 +0300
> Date: Sun, 19 Jul 2020 22:01:00 +0300
> From: Eli Zaretskii <email@example.com>
> Cc: firstname.lastname@example.org
> > From: Mathias Dahl <email@example.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
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
> +(defun abbrev--suggest-shortest-abbrev (new current)
> + "Return the shortest abbrev.
> +NEW and CURRENT are cons cells where the `car' is the expansion
> +(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
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.