bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Rudolf Schlatte]

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

Tags: patch

Hi,

Currently, yaml-ts-mode fills comments and block scalars (multi-line
text literals) as expected, but re-fills the whole file when point is
outside of either of these constructs.  Since yaml line breaks and
whitespace are significant, I'd say that this is never the correct
behavior.

This patch against current master inhibits M-q (fill-paragraph) outside
of comments and block scalars.  In my tests default fill-paragraph
worked as expected both with and without justify, correctly detecting
comment and block literal boundaries, so I did not preserve the previous
code in `yaml-ts-mode--fill-paragraph'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-fill-yaml-except-comments-and-block-scalars.patch --]
[-- Type: text/patch, Size: 2220 bytes --]

From 003e9f0dbd20059dbf49761a7537658947a98d55 Mon Sep 17 00:00:00 2001
From: Rudolf Schlatte <rudi@Cafeolix.local>
Date: Sun, 28 Jan 2024 13:54:35 +0100
Subject: [PATCH] Don't fill yaml except comments and block scalars.

Indentation and line breaks are significant syntax, so only fill
reflowable nodes.

* lisp/textmodes/yaml-ts-mode.el (yaml-ts-mode--fill-paragraph): Check
if point is inside a comment or block scalar, do not fill otherwise.
---
 lisp/textmodes/yaml-ts-mode.el | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/lisp/textmodes/yaml-ts-mode.el b/lisp/textmodes/yaml-ts-mode.el
index c0185457bc2..301d7ba03a9 100644
--- a/lisp/textmodes/yaml-ts-mode.el
+++ b/lisp/textmodes/yaml-ts-mode.el
@@ -120,25 +120,17 @@ yaml-ts-mode--font-lock-settings
    '((ERROR) @font-lock-warning-face))
   "Tree-sitter font-lock settings for `yaml-ts-mode'.")
 
-(defun yaml-ts-mode--fill-paragraph (&optional justify)
+(defun yaml-ts-mode--fill-paragraph (&optional _justify)
   "Fill paragraph.
-Behaves like `fill-paragraph', but respects block node
-boundaries.  JUSTIFY is passed to `fill-paragraph'."
-  (interactive "*P")
-  (save-restriction
-    (widen)
-    (let ((node (treesit-node-at (point))))
-      (when (string= "block_scalar" (treesit-node-type node))
-        (let* ((start (treesit-node-start node))
-               (end (treesit-node-end node))
-               (start-marker (point-marker))
-               (fill-paragraph-function nil))
-          (save-excursion
-            (goto-char start)
-            (forward-line)
-            (move-marker start-marker (point))
-            (narrow-to-region (point) end))
-          (fill-region start-marker end justify))))))
+Hand over to `fill-paragraph' if point is inside a comment or
+block scalar; do nothing otherwise."
+  (let ((node (treesit-node-at (point)))
+        (fillable-types '("block_scalar" "comment")))
+    (if (member (treesit-node-type node) fillable-types)
+        ;; Explicitly return these two specific values; see
+        ;; `fill-paragraph-function' documentation.
+        nil
+      t)))
 
 ;;;###autoload
 (define-derived-mode yaml-ts-mode text-mode "YAML"
-- 
2.43.0

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Rudolf Schlatte]

Rudolf Schlatte <rudi@constantly.at> writes:

> In my tests default fill-paragraph
> worked as expected both with and without justify, correctly detecting
> comment and block literal boundaries, so I did not preserve the previous
> code in `yaml-ts-mode--fill-paragraph'.

Actually there is a change in behavior: if the block scalar contains
empty lines, hence having multiple paragraphs, current master fills all
paragraphs, while after the patch only the current paragraph is filled.

I.e., in the following yaml file:
---
text_section: |
  This is a paragraph of text.

  This is a second paragraph.
some_numeric_value: 1
---
currently lines 2-4 get filled if point is inside these lines, whereas
after the patch only line 2 or line 4 are filled, depending on the
position of point.

I find that I prefer the new behavior, and hope that this change is
acceptable.

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Randy Taylor]

On Sunday, January 28th, 2024 at 08:15, Rudolf Schlatte <rudi@constantly.at> wrote:
> 
> 
> Tags: patch
> 
> Hi,
> 
> Currently, yaml-ts-mode fills comments and block scalars (multi-line
> text literals) as expected, but re-fills the whole file when point is
> outside of either of these constructs. Since yaml line breaks and
> whitespace are significant, I'd say that this is never the correct
> behavior.
> 
> This patch against current master inhibits M-q (fill-paragraph) outside
> of comments and block scalars. In my tests default fill-paragraph
> worked as expected both with and without justify, correctly detecting
> comment and block literal boundaries, so I did not preserve the previous
> code in `yaml-ts-mode--fill-paragraph'.

Thanks for working on this.

The previous implementation (see bug#68226) provided an example where:
foo: |
  line-one
  line-two

Would become:
foo: | line-one line-two

When it should be:
foo: |
  line-one line-two

Your patch undoes this fix.

I think I also ran into another bug using your patch: when inside a block scalar, for example, running fill-paragraph will re-fill the whole file (or parts of it at least).

I agree that we shouldn't try to fill anything that isn't a comment or block scalar, and I also like the change in your other message where only the paragraph that point is on gets filled.

BTW would it be possible to add tests for these? See `c-ts-mode-tests.el', specifically `c-ts-mode-test-filling', for inspiration.

Graham, you added `yaml-ts-mode--fill-paragraph' - what are your thoughts? See also Rudolf's other message.

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Rudolf Schlatte]

Randy Taylor <dev@rjt.dev> writes:

> On Sunday, January 28th, 2024 at 08:15, Rudolf Schlatte <rudi@constantly.at> wrote:
>> 
>> Hi,
>> 
>> Currently, yaml-ts-mode fills comments and block scalars (multi-line
>> text literals) as expected, but re-fills the whole file when point is
>> outside of either of these constructs. Since yaml line breaks and
>> whitespace are significant, I'd say that this is never the correct
>> behavior.
>> 
>> This patch against current master inhibits M-q (fill-paragraph) outside
>> of comments and block scalars. In my tests default fill-paragraph
>> worked as expected both with and without justify, correctly detecting
>> comment and block literal boundaries, so I did not preserve the previous
>> code in `yaml-ts-mode--fill-paragraph'.
>
> Thanks for working on this.
>
> The previous implementation (see bug#68226) provided an example where:
> foo: |
>   line-one
>   line-two
>
> Would become:
> foo: | line-one line-two
>
> When it should be:
> foo: |
>   line-one line-two
>
> Your patch undoes this fix.
>
> I think I also ran into another bug using your patch: when inside a block
> scalar, for example, running fill-paragraph will re-fill the whole file (or
> parts of it at least).
>
> I agree that we shouldn't try to fill anything that isn't a comment or block
> scalar, and I also like the change in your other message where only the
> paragraph that point is on gets filled.
>
> BTW would it be possible to add tests for these? See `c-ts-mode-tests.el',
> specifically `c-ts-mode-test-filling', for inspiration.
>
> Graham, you added `yaml-ts-mode--fill-paragraph' - what are your thoughts? See also Rudolf's other message.

Hello Randy, thanks for having a look!  Could you tell me which
tree-sitter grammar you are using?  I'm asking because with the grammar
from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
describe.

Needless to say, please don't install the patch before I have debugged
this.  :)

Best, Rudi

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Randy Taylor]

On Monday, January 29th, 2024 at 03:20, Rudolf Schlatte <rudi@constantly.at> wrote:
> 
> Hello Randy, thanks for having a look! Could you tell me which
> tree-sitter grammar you are using? I'm asking because with the grammar
> from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
> describe.

That's the one I'm using.

Here are some examples I see when using emacs -Q:

a:
    b: 4
foo: |
  l<POINT>ine-one

  line-two

Place point where <POINT> is (and remove that text of course). Then run M-x fill-paragraph. This is what I see as a result:

a: b: 4 foo: | line-one

  line-two

And with the example from bug#68226:
foo: |
  l<POINT>ine-one
  line-two

After M-x fill-paragraph I see:
foo: | line-one line-two

Hopefully these reproduce for you too.

> 
> Needless to say, please don't install the patch before I have debugged
> this. :)
> 
> Best, Rudi

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Graham Marlow]

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

> Hello Randy, thanks for having a look! Could you tell me which
> tree-sitter grammar you are using? I'm asking because with the grammar
> from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
> describe.

For the record I'm also using this grammar.

Looking at the patch, what do you think about retaining the existing 
behavior (so block_scalars still fill correctly) while inhibiting 
fill_paragraph for everything else as suggested? Originally I retained 
the existing behavior of fill-paragraph just to limit the number of 
things changed by the patch, not because it was working properly. I 
think blocking the call to fill-paragraph for non-block/comment nodes 
makes sense.

I attached a patch w/ my edits, but it just swaps the when to and if, 
accepts the comment node type for filling, and returns t to avoid 
calling fill-paragraph for other nodes.

[-- Attachment #2: 0001-Inhibit-fill-paragraph-outside-of-blocks-comments.patch --]
[-- Type: text/plain, Size: 1476 bytes --]

From 5cc568f6cc7b237337b973aa9f64f6c97ab7bca8 Mon Sep 17 00:00:00 2001
From: Graham Marlow <graham@mgmarlow.com>
Date: Mon, 29 Jan 2024 17:16:04 -0800
Subject: [PATCH] Inhibit fill-paragraph outside of blocks/comments

* lisp/textmodes/yaml-ts-mode.el (yaml-ts-mode--fill-paragraph): Avoid
  fill-paragraph when outside of block_scalar or comment nodes.
---
 lisp/textmodes/yaml-ts-mode.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/textmodes/yaml-ts-mode.el b/lisp/textmodes/yaml-ts-mode.el
index c0185457bc2..a8cb504ef03 100644
--- a/lisp/textmodes/yaml-ts-mode.el
+++ b/lisp/textmodes/yaml-ts-mode.el
@@ -128,7 +128,7 @@ boundaries.  JUSTIFY is passed to `fill-paragraph'."
   (save-restriction
     (widen)
     (let ((node (treesit-node-at (point))))
-      (when (string= "block_scalar" (treesit-node-type node))
+      (if (member (treesit-node-type node) '("block_scalar" "comment"))
         (let* ((start (treesit-node-start node))
                (end (treesit-node-end node))
                (start-marker (point-marker))
@@ -138,7 +138,8 @@ boundaries.  JUSTIFY is passed to `fill-paragraph'."
             (forward-line)
             (move-marker start-marker (point))
             (narrow-to-region (point) end))
-          (fill-region start-marker end justify))))))
+          (fill-region start-marker end justify))
+        t))))
 
 ;;;###autoload
 (define-derived-mode yaml-ts-mode text-mode "YAML"
-- 
2.42.0.windows.1

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Rudolf Schlatte]

Graham Marlow <graham@mgmarlow.com> writes:

>> Hello Randy, thanks for having a look! Could you tell me which
>> tree-sitter grammar you are using? I'm asking because with the grammar
>> from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
>> describe.
>
> For the record I'm also using this grammar.
>
> Looking at the patch, what do you think about retaining the existing behavior
> (so block_scalars still fill correctly) while inhibiting fill_paragraph for
> everything else as suggested? Originally I retained the existing behavior of
> fill-paragraph just to limit the number of things changed by the patch, not
> because it was working properly. I think blocking the call to fill-paragraph
> for non-block/comment nodes makes sense.

I'm suddenly a bit stressed for time and can't investigate properly for
the next few days why my proposed patch misbehaves under some
circumstances..  And your patch fixes the misbehavior with a minimum of
changes, so I think it's the one that should go in.

Thanks to both of you for working on this!

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Randy Taylor]

On Monday, January 29th, 2024 at 20:20, Graham Marlow <graham@mgmarlow.com> wrote:
> 
> 
> > Hello Randy, thanks for having a look! Could you tell me which
> 
> > tree-sitter grammar you are using? I'm asking because with the grammar
> > from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
> > describe.
> 
> 
> For the record I'm also using this grammar.
> 
> Looking at the patch, what do you think about retaining the existing
> behavior (so block_scalars still fill correctly) while inhibiting
> fill_paragraph for everything else as suggested? Originally I retained
> the existing behavior of fill-paragraph just to limit the number of
> things changed by the patch, not because it was working properly. I
> think blocking the call to fill-paragraph for non-block/comment nodes
> makes sense.
> 
> I attached a patch w/ my edits, but it just swaps the when to and if,
> accepts the comment node type for filling, and returns t to avoid
> calling fill-paragraph for other nodes.

Thanks Graham, the patch looks good to me.

Would someone please install it on master? Thanks in advance.

bug#68781: [PATCH] Don't fill yaml except comments and block scalars.

[Eli Zaretskii]

> Cc: Rudolf Schlatte <rudi@constantly.at>, 68781@debbugs.gnu.org
> Date: Tue, 30 Jan 2024 19:25:07 +0000
> From: Randy Taylor <dev@rjt.dev>
> 
> On Monday, January 29th, 2024 at 20:20, Graham Marlow <graham@mgmarlow.com> wrote:
> > 
> > 
> > > Hello Randy, thanks for having a look! Could you tell me which
> > 
> > > tree-sitter grammar you are using? I'm asking because with the grammar
> > > from https://github.com/ikatyang/tree-sitter-yaml I don't see what you
> > > describe.
> > 
> > 
> > For the record I'm also using this grammar.
> > 
> > Looking at the patch, what do you think about retaining the existing
> > behavior (so block_scalars still fill correctly) while inhibiting
> > fill_paragraph for everything else as suggested? Originally I retained
> > the existing behavior of fill-paragraph just to limit the number of
> > things changed by the patch, not because it was working properly. I
> > think blocking the call to fill-paragraph for non-block/comment nodes
> > makes sense.
> > 
> > I attached a patch w/ my edits, but it just swaps the when to and if,
> > accepts the comment node type for filling, and returns t to avoid
> > calling fill-paragraph for other nodes.
> 
> Thanks Graham, the patch looks good to me.
> 
> Would someone please install it on master? Thanks in advance.

Thanks, done, and closing the bug.