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