GNU bug report logs - #68781
[PATCH] Don't fill yaml except comments and block scalars.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68781 in the body.
You can then email your comments to 68781 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rudolf Schlatte <rudi <at> constantly.at>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Don't fill yaml except comments and block scalars.
Date: Sun, 28 Jan 2024 14:15:58 +0100

[Message part 1 (text/plain, inline)]

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

[0001-Don-t-fill-yaml-except-comments-and-block-scalars.patch (text/patch, attachment)]

Message #8 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Rudolf Schlatte <rudi <at> constantly.at>
To: 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Sun, 28 Jan 2024 15:52:50 +0100

Rudolf Schlatte <rudi <at> 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.

Message #11 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Randy Taylor <dev <at> rjt.dev>
To: Rudolf Schlatte <rudi <at> constantly.at>
Cc: "graham <at> mgmarlow.com" <graham <at> mgmarlow.com>, 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Mon, 29 Jan 2024 03:47:53 +0000

On Sunday, January 28th, 2024 at 08:15, Rudolf Schlatte <rudi <at> 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.

Message #14 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Rudolf Schlatte <rudi <at> constantly.at>
To: Randy Taylor <dev <at> rjt.dev>
Cc: "graham <at> mgmarlow.com" <graham <at> mgmarlow.com>, 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Mon, 29 Jan 2024 09:20:04 +0100

Randy Taylor <dev <at> rjt.dev> writes:

> On Sunday, January 28th, 2024 at 08:15, Rudolf Schlatte <rudi <at> 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

Message #17 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Randy Taylor <dev <at> rjt.dev>
To: Rudolf Schlatte <rudi <at> constantly.at>
Cc: "graham <at> mgmarlow.com" <graham <at> mgmarlow.com>, 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Mon, 29 Jan 2024 14:08:01 +0000

On Monday, January 29th, 2024 at 03:20, Rudolf Schlatte <rudi <at> 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

Message #20 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Graham Marlow <graham <at> mgmarlow.com>
To: Randy Taylor <dev <at> rjt.dev>, Rudolf Schlatte <rudi <at> constantly.at>
Cc: 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Mon, 29 Jan 2024 17:20:04 -0800

[Message part 1 (text/plain, inline)]

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

[0001-Inhibit-fill-paragraph-outside-of-blocks-comments.patch (text/plain, attachment)]

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rudolf Schlatte <rudi <at> constantly.at>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Tue, 30 Jan 2024 11:15:34 +0100

Graham Marlow <graham <at> 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!

Message #26 received at 68781 <at> debbugs.gnu.org (full text, mbox):

From: Randy Taylor <dev <at> rjt.dev>
To: Graham Marlow <graham <at> mgmarlow.com>
Cc: Rudolf Schlatte <rudi <at> constantly.at>, 68781 <at> debbugs.gnu.org
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Tue, 30 Jan 2024 19:25:07 +0000

On Monday, January 29th, 2024 at 20:20, Graham Marlow <graham <at> 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.

Message #31 received at 68781-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Randy Taylor <dev <at> rjt.dev>
Cc: 68781-done <at> debbugs.gnu.org, rudi <at> constantly.at, graham <at> mgmarlow.com
Subject: Re: bug#68781: [PATCH] Don't fill yaml except comments and block
 scalars.
Date: Thu, 01 Feb 2024 12:31:30 +0200

> Cc: Rudolf Schlatte <rudi <at> constantly.at>, 68781 <at> debbugs.gnu.org
> Date: Tue, 30 Jan 2024 19:25:07 +0000
> From: Randy Taylor <dev <at> rjt.dev>
> 
> On Monday, January 29th, 2024 at 20:20, Graham Marlow <graham <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.