bug#24622: 26.0.50; lisp-fill-paragraph broken

bug#24622: 26.0.50; lisp-fill-paragraph broken

[martin rudalics]

This commit

commit 866e3c050fe64fee81f29a335a50a11b2562422e
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Thu Apr 28 12:05:15 2016 +0200

     Don't consider colons to be paragraphs starting chars in strings

     * lisp/emacs-lisp/lisp-mode.el (lisp-fill-paragraph): Don't
     consider colons to start paragraphs in (doc) strings
     (bug#7751).

breaks filling doc-strings of defcustoms.  As an example consider the
defcustom of ‘window-min-height’ in window.el:


(defcustom window-min-height 4
   "The minimum total height, in lines, of any window.
The value has to accommodate one text line, a mode and header
line, a horizontal scroll bar and a bottom divider, if present.
A value less than `window-safe-min-height' is ignored.  The value
of this variable is honored when windows are resized or split.

Applications should never rebind this variable.  To resize a
window to a height less than the one specified here, an
application should instead call `window-resize' with a non-nil
IGNORE argument.  In order to have `split-window' make a window
shorter, explicitly specify the SIZE argument of that function."
   :type 'integer
   :version "24.1"
   :group 'windows)


Put point at the beginning of the last line of the doc-string and do

M-: (fill-paragraph)

This gets me here


(defcustom window-min-height 4
   "The minimum total height, in lines, of any window.
The value has to accommodate one text line, a mode and header
line, a horizontal scroll bar and a bottom divider, if present.
A value less than `window-safe-min-height' is ignored.  The value
of this variable is honored when windows are resized or split.

Applications should never rebind this variable.  To resize a
window to a height less than the one specified here, an
application should instead call `window-resize' with a non-nil
IGNORE argument.  In order to have `split-window' make a window
shorter, explicitly specify the SIZE argument of that function."
:type 'integer :version "24.1" :group 'windows)


martin

bug#24622: 26.0.50; lisp-fill-paragraph broken

[Alex]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

martin rudalics <rudalics@gmx.at> writes:

> This commit
>
> commit 866e3c050fe64fee81f29a335a50a11b2562422e
> Author: Lars Ingebrigtsen <larsi@gnus.org>
> Date:   Thu Apr 28 12:05:15 2016 +0200
>
>     Don't consider colons to be paragraphs starting chars in strings
>
>     * lisp/emacs-lisp/lisp-mode.el (lisp-fill-paragraph): Don't
>     consider colons to start paragraphs in (doc) strings
>     (bug#7751).
>
> breaks filling doc-strings of defcustoms.  As an example consider the
> defcustom of ‘window-min-height’ in window.el:

I've included a diff below that appears to solve both bug#7751 and this
one, though I can't guarantee that it doesn't blow something else up.
Does anyone have any complaints?

In any case, I think this bug should be solved before 26.1 is released.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: paragraph --]
[-- Type: text/x-diff, Size: 1620 bytes --]

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index fd12635d85..93435e1b4b 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1267,7 +1267,8 @@ lisp-fill-paragraph
       ;; case).  The `;' and `:' stop the paragraph being filled at following
       ;; comment lines and at keywords (e.g., in `defcustom').  Left parens are
       ;; escaped to keep font-locking, filling, & paren matching in the source
-      ;; file happy.
+      ;; file happy.  The `:' must be preceded by whitespace so that keywords
+      ;; inside of the docstring don't start new paragraphs (Bug#7751).
       ;;
       ;; `paragraph-separate': A clever regexp distinguishes the first line of
       ;; a docstring and identifies it as a paragraph separator, so that it
@@ -1280,13 +1281,7 @@ lisp-fill-paragraph
       ;; `emacs-lisp-docstring-fill-column' if that value is an integer.
       (let ((paragraph-start
              (concat paragraph-start
-                     (format "\\|\\s-*\\([(;%s\"]\\|`(\\|#'(\\)"
-                             ;; If we're inside a string (like the doc
-                             ;; string), don't consider a colon to be
-                             ;; a paragraph-start character.
-                             (if (nth 3 (syntax-ppss))
-                                 ""
-                               ":"))))
+                     "\\|\\s-*\\([(;\"]\\|\\s-:\\|`(\\|#'(\\)"))
 	    (paragraph-separate
 	     (concat paragraph-separate "\\|\\s-*\".*[,\\.]$"))
             (fill-column (if (and (integerp emacs-lisp-docstring-fill-column)

bug#24622: 26.0.50; lisp-fill-paragraph broken

[martin rudalics]

 >>      Don't consider colons to be paragraphs starting chars in strings
 >>
 >>      * lisp/emacs-lisp/lisp-mode.el (lisp-fill-paragraph): Don't
 >>      consider colons to start paragraphs in (doc) strings
 >>      (bug#7751).
 >>
 >> breaks filling doc-strings of defcustoms.  As an example consider the
 >> defcustom of ‘window-min-height’ in window.el:
 >
 > I've included a diff below that appears to solve both bug#7751 and this
 > one, though I can't guarantee that it doesn't blow something else up.
 > Does anyone have any complaints?
 >
 > In any case, I think this bug should be solved before 26.1 is released.

I obviously think so too.  Lars, how do we proceed here?  I'd vote
for checking in Alex's patch.

martin

bug#24622: 26.0.50; lisp-fill-paragraph broken

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Date: Thu, 12 Oct 2017 20:23:01 -0600
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 24622@debbugs.gnu.org
> 
> > This commit
> >
> > commit 866e3c050fe64fee81f29a335a50a11b2562422e
> > Author: Lars Ingebrigtsen <larsi@gnus.org>
> > Date:   Thu Apr 28 12:05:15 2016 +0200
> >
> >     Don't consider colons to be paragraphs starting chars in strings
> >
> >     * lisp/emacs-lisp/lisp-mode.el (lisp-fill-paragraph): Don't
> >     consider colons to start paragraphs in (doc) strings
> >     (bug#7751).
> >
> > breaks filling doc-strings of defcustoms.  As an example consider the
> > defcustom of ‘window-min-height’ in window.el:
> 
> I've included a diff below that appears to solve both bug#7751 and this
> one, though I can't guarantee that it doesn't blow something else up.
> Does anyone have any complaints?
> 
> In any case, I think this bug should be solved before 26.1 is released.

Since there were no more comments, please push to the release branch.

Thanks.

bug#24622: 26.0.50; lisp-fill-paragraph broken

[Alex]

[-- Attachment #1: Type: text/plain, Size: 186 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Since there were no more comments, please push to the release branch.
>
> Thanks.

I added some tests to the patch. Is the following acceptable?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fill --]
[-- Type: text/x-diff, Size: 3761 bytes --]

From f3bdd14347be5ba2e8e8ca91a9b056b768398f73 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Sun, 22 Oct 2017 01:46:05 -0600
Subject: [PATCH] Don't fill keywords after Emacs Lisp docstring

This approach does mean that keywords that have spaces before them
inside of docstrings aren't filled, but I think this is should be fine
until Bug#28937 is fixed.

* lisp/emacs-lisp/lisp-mode.el (lisp-fill-paragraph): Add a colon to
paragraph-start unconditionally, but require that it proceeds at least
one space.  (Bug#24622)
* test/lisp/emacs-lisp/lisp-tests.el: New tests for Bug#24622 and
Bug#7751.
---
 lisp/emacs-lisp/lisp-mode.el       | 11 +++--------
 test/lisp/emacs-lisp/lisp-tests.el | 31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index fd12635d85..93435e1b4b 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1267,7 +1267,8 @@ lisp-fill-paragraph
       ;; case).  The `;' and `:' stop the paragraph being filled at following
       ;; comment lines and at keywords (e.g., in `defcustom').  Left parens are
       ;; escaped to keep font-locking, filling, & paren matching in the source
-      ;; file happy.
+      ;; file happy.  The `:' must be preceded by whitespace so that keywords
+      ;; inside of the docstring don't start new paragraphs (Bug#7751).
       ;;
       ;; `paragraph-separate': A clever regexp distinguishes the first line of
       ;; a docstring and identifies it as a paragraph separator, so that it
@@ -1280,13 +1281,7 @@ lisp-fill-paragraph
       ;; `emacs-lisp-docstring-fill-column' if that value is an integer.
       (let ((paragraph-start
              (concat paragraph-start
-                     (format "\\|\\s-*\\([(;%s\"]\\|`(\\|#'(\\)"
-                             ;; If we're inside a string (like the doc
-                             ;; string), don't consider a colon to be
-                             ;; a paragraph-start character.
-                             (if (nth 3 (syntax-ppss))
-                                 ""
-                               ":"))))
+                     "\\|\\s-*\\([(;\"]\\|\\s-:\\|`(\\|#'(\\)"))
 	    (paragraph-separate
 	     (concat paragraph-separate "\\|\\s-*\".*[,\\.]$"))
             (fill-column (if (and (integerp emacs-lisp-docstring-fill-column)
diff --git a/test/lisp/emacs-lisp/lisp-tests.el b/test/lisp/emacs-lisp/lisp-tests.el
index ae1302bdce..6fb2f8adc9 100644
--- a/test/lisp/emacs-lisp/lisp-tests.el
+++ b/test/lisp/emacs-lisp/lisp-tests.el
@@ -589,5 +589,36 @@ elisp-tests-with-temp-buffer
     (should (= (point) before))
     (should (= (mark) after))))
 
+(ert-deftest lisp-fill-paragraph-colon ()
+  "Keywords below Emacs Lisp docstrings should not be filled (Bug#24622).
+Keywords inside docstrings should be filled (Bug#7751)."
+  (elisp-tests-with-temp-buffer
+      "
+\(defcustom custom value
+  \"First\n
+Second\n
+=!inside=Third line\"
+  =!keywords=:type 'sexp
+  :version \"26.1\"
+  :group 'lisp-tests)"
+    (goto-char inside)
+    (fill-paragraph)
+    (goto-char keywords)
+    (beginning-of-line)
+    (should (looking-at "  :type 'sexp\n  :version \"26.1\"\n  :")))
+  (elisp-tests-with-temp-buffer
+      "
+\(defun foo ()
+  \"Summary.
+=!inside=Testing keywords: :one :two :three\"
+  (body))" ; FIXME: Remove parens around body to test bug#28937 once it's fixed
+    (goto-char inside)
+    (let ((emacs-lisp-docstring-fill-column 30))
+      (fill-paragraph))
+    (forward-line)
+    (should (looking-at ":three"))
+    (end-of-line)
+    (should-not (eq (preceding-char) ?\)))))
+
 (provide 'lisp-tests)
 ;;; lisp-tests.el ends here
-- 
2.14.2


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]


I added in a reference to bug#28937, which I just reported. Fixing that
would fix also fix this bug, but that solution might not be ready for
Emacs 26.

bug#24622: 26.0.50; lisp-fill-paragraph broken

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: larsi@gnus.org,  24622@debbugs.gnu.org
> Date: Sun, 22 Oct 2017 02:10:47 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Since there were no more comments, please push to the release branch.
> >
> > Thanks.
> 
> I added some tests to the patch. Is the following acceptable?

Yes, thanks.

> I added in a reference to bug#28937, which I just reported. Fixing that
> would fix also fix this bug, but that solution might not be ready for
> Emacs 26.

If you intend to solve this differently on master, and you think you
will do that soon, you can indicate that this patch should not be
merged to master.

bug#24622: 26.0.50; lisp-fill-paragraph broken

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Cc: larsi@gnus.org,  24622@debbugs.gnu.org
>> Date: Sun, 22 Oct 2017 02:10:47 -0600
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Since there were no more comments, please push to the release branch.
>> >
>> > Thanks.
>> 
>> I added some tests to the patch. Is the following acceptable?
>
> Yes, thanks.

Okay, pushed to emacs-26 as a012ec766c9d9bac0a56e814589a4b3b93311c28.

>> I added in a reference to bug#28937, which I just reported. Fixing that
>> would fix also fix this bug, but that solution might not be ready for
>> Emacs 26.
>
> If you intend to solve this differently on master, and you think you
> will do that soon, you can indicate that this patch should not be
> merged to master.

Thanks for the tip, but I likely won't get to it soon. I think this
should be merged to master in the meantime.

bug#24622: 26.0.50; lisp-fill-paragraph broken

[martin rudalics]

 > Okay, pushed to emacs-26 as a012ec766c9d9bac0a56e814589a4b3b93311c28.

Thanks Alex for fixing this.

martin