GNU bug report logs - #60128
30.0.50; [PATCH]: Add treesit-transpose-sexps

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 60128 in the body.
You can then email your comments to 60128 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: Theodor Thornhill <theo <at> thornhill.no>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Fri, 16 Dec 2022 21:04:28 +0100

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

Hi there!

Attached is a patch that enables transpose-sexps for tree-sitter enabled
modes.

This function will swap the node _before_ node-at-point with the node
_after_, so it would do something like:

       foo a|nd bar => bar and foo|

or
       foo(a + 4,| y + c * b, b, d); => foo(y + c * b, a + 4|, b, d);

It will _not_ try to swap things that are not siblings.  I think that
makes sense in the case of non-lisp languages, since _most_ places you
can transpose-sexps you will end up with broken code.

What do you think?  I think this makes sense on the master branch, not
release.

Theo

[0001-Add-treesit-transpose-sexps.patch (text/x-diff, attachment)]

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

From: Theodor Thornhill <theo <at> thornhill.no>
To: 60128 <at> debbugs.gnu.org
Cc: casouri <at> gmail.com, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Sat, 17 Dec 2022 13:52:30 +0100

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

Theodor Thornhill <theo <at> thornhill.no> writes:

> Hi there!
>
> Attached is a patch that enables transpose-sexps for tree-sitter enabled
> modes.
>
> This function will swap the node _before_ node-at-point with the node
> _after_, so it would do something like:
>
>        foo a|nd bar => bar and foo|
>
> or
>        foo(a + 4,| y + c * b, b, d); => foo(y + c * b, a + 4|, b, d);
>
> It will _not_ try to swap things that are not siblings.  I think that
> makes sense in the case of non-lisp languages, since _most_ places you
> can transpose-sexps you will end up with broken code.
>

from 'transpose-subr-1':

  (if (> (cdr pos1) (car pos2)) (error "Don't have two things to
  transpose"))

I added this hack into the function in the patch, but I think that
triggering an error is too much.

    ;; Hack to trigger the error message in `transpose-subr-1' when we
    ;; don't have siblings to swap.
    (list (cons 0 1) (cons 0 1))))

I guess I could just follow suit in my function and do like in the
following patch:

Theo

[0001-Add-treesit-transpose-sexps.patch (text/x-diff, attachment)]

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

From: Theodor Thornhill <theo <at> thornhill.no>
To: 60128 <at> debbugs.gnu.org
Cc: casouri <at> gmail.com, eliz <at> gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Mon, 26 Dec 2022 21:53:57 +0100

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

Theodor Thornhill <theo <at> thornhill.no> writes:

> Theodor Thornhill <theo <at> thornhill.no> writes:
>
>> Hi there!
>>
>> Attached is a patch that enables transpose-sexps for tree-sitter enabled
>> modes.
>>
>> This function will swap the node _before_ node-at-point with the node
>> _after_, so it would do something like:
>>
>>        foo a|nd bar => bar and foo|
>>
>> or
>>        foo(a + 4,| y + c * b, b, d); => foo(y + c * b, a + 4|, b, d);
>>
>> It will _not_ try to swap things that are not siblings.  I think that
>> makes sense in the case of non-lisp languages, since _most_ places you
>> can transpose-sexps you will end up with broken code.
>>
>
> from 'transpose-subr-1':
>
>   (if (> (cdr pos1) (car pos2)) (error "Don't have two things to
>   transpose"))
>
> I added this hack into the function in the patch, but I think that
> triggering an error is too much.
>
>     ;; Hack to trigger the error message in `transpose-subr-1' when we
>     ;; don't have siblings to swap.
>     (list (cons 0 1) (cons 0 1))))
>
> I guess I could just follow suit in my function and do like in the
> following patch:
>
> Theo


Considering there is both a bug-report _and_ a discussion around this I
guess the best idea is to add the patch to this bug report, and continue
discussing this in the report rather than emacs-devel?

What do you think about this patch?

(copied from emacs-devel):
It feels a little iffy how to handle the separate return values, but it
works.  I'd be super happy for some feedback on how to best solve that,
though :)

Also, I made the treesit-transpose-sexps a little better imo, in that we
only find named nodes to swap, but use every available node for the
entry. We rarely, if ever want to swap the unnamed nodes.

Eli, does this require a NEWS entry or more documentation?

Theo

[0001-Add-treesit-transpose-sexps-bug-60128.patch (text/x-diff, attachment)]

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

From: Yuan Fu <casouri <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 60128 <at> debbugs.gnu.org, eliz <at> gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Mon, 26 Dec 2022 13:26:54 -0800

> On Dec 26, 2022, at 12:53 PM, Theodor Thornhill <theo <at> thornhill.no> wrote:
> 
> Theodor Thornhill <theo <at> thornhill.no> writes:
> 
>> Theodor Thornhill <theo <at> thornhill.no> writes:
>> 
>>> Hi there!
>>> 
>>> Attached is a patch that enables transpose-sexps for tree-sitter enabled
>>> modes.
>>> 
>>> This function will swap the node _before_ node-at-point with the node
>>> _after_, so it would do something like:
>>> 
>>>       foo a|nd bar => bar and foo|
>>> 
>>> or
>>>       foo(a + 4,| y + c * b, b, d); => foo(y + c * b, a + 4|, b, d);
>>> 
>>> It will _not_ try to swap things that are not siblings.  I think that
>>> makes sense in the case of non-lisp languages, since _most_ places you
>>> can transpose-sexps you will end up with broken code.
>>> 
>> 
>> from 'transpose-subr-1':
>> 
>>  (if (> (cdr pos1) (car pos2)) (error "Don't have two things to
>>  transpose"))
>> 
>> I added this hack into the function in the patch, but I think that
>> triggering an error is too much.
>> 
>>    ;; Hack to trigger the error message in `transpose-subr-1' when we
>>    ;; don't have siblings to swap.
>>    (list (cons 0 1) (cons 0 1))))
>> 
>> I guess I could just follow suit in my function and do like in the
>> following patch:
>> 
>> Theo
> 
> 
> Considering there is both a bug-report _and_ a discussion around this I
> guess the best idea is to add the patch to this bug report, and continue
> discussing this in the report rather than emacs-devel?
> 
> What do you think about this patch?
> 
> (copied from emacs-devel):
> It feels a little iffy how to handle the separate return values, but it
> works.  I'd be super happy for some feedback on how to best solve that,
> though :)

By separate return values, do you mean the function returns a cons of cons? It seems fine to me. Though I think the docstring could be more specific. Like saying return a cons (REGION . REGION), where REGION is (BEG . END), where BEG and END...

> 
> Also, I made the treesit-transpose-sexps a little better imo, in that we
> only find named nodes to swap, but use every available node for the
> entry. We rarely, if ever want to swap the unnamed nodes.
> 
> Eli, does this require a NEWS entry or more documentation?

IMHO a backend/helper function shouldn’t signal a user-error, it’s better to return nil and let the command to signal errors.

Yuan

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

From: Theodor Thornhill <theo <at> thornhill.no>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 60128 <at> debbugs.gnu.org, eliz <at> gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Mon, 26 Dec 2022 23:37:40 +0100

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

Yuan Fu <casouri <at> gmail.com> writes:

>> On Dec 26, 2022, at 12:53 PM, Theodor Thornhill <theo <at> thornhill.no> wrote:
>> 
>> Theodor Thornhill <theo <at> thornhill.no> writes:
>> 
>>> Theodor Thornhill <theo <at> thornhill.no> writes:
>>> 
>>>> Hi there!
>>>> 
>>>> Attached is a patch that enables transpose-sexps for tree-sitter enabled
>>>> modes.
>>>> 
>>>> This function will swap the node _before_ node-at-point with the node
>>>> _after_, so it would do something like:
>>>> 
>>>>       foo a|nd bar => bar and foo|
>>>> 
>>>> or
>>>>       foo(a + 4,| y + c * b, b, d); => foo(y + c * b, a + 4|, b, d);
>>>> 
>>>> It will _not_ try to swap things that are not siblings.  I think that
>>>> makes sense in the case of non-lisp languages, since _most_ places you
>>>> can transpose-sexps you will end up with broken code.
>>>> 
>>> 
>>> from 'transpose-subr-1':
>>> 
>>>  (if (> (cdr pos1) (car pos2)) (error "Don't have two things to
>>>  transpose"))
>>> 
>>> I added this hack into the function in the patch, but I think that
>>> triggering an error is too much.
>>> 
>>>    ;; Hack to trigger the error message in `transpose-subr-1' when we
>>>    ;; don't have siblings to swap.
>>>    (list (cons 0 1) (cons 0 1))))
>>> 
>>> I guess I could just follow suit in my function and do like in the
>>> following patch:
>>> 
>>> Theo
>> 
>> 
>> Considering there is both a bug-report _and_ a discussion around this I
>> guess the best idea is to add the patch to this bug report, and continue
>> discussing this in the report rather than emacs-devel?
>> 
>> What do you think about this patch?
>> 
>> (copied from emacs-devel):
>> It feels a little iffy how to handle the separate return values, but it
>> works.  I'd be super happy for some feedback on how to best solve that,
>> though :)
>
> By separate return values, do you mean the function returns a cons of
> cons? It seems fine to me. Though I think the docstring could be more
> specific. Like saying return a cons (REGION . REGION), where REGION is
> (BEG . END), where BEG and END...
>

I updated the patch adressing your comment.  The current "protocol" used
on the master branch implies that the regions are calculated by actually
moving inside the buffer and running (point).  Because we don't need
that with the ast in hand I'm trying to support a new protocol, where we
just supply the data we need.  It's a little difficult to "surgically"
modify the behavior, so I'm very open to suggestions, but I believe it
should work properly as the patch stands.

>> 
>> Also, I made the treesit-transpose-sexps a little better imo, in that we
>> only find named nodes to swap, but use every available node for the
>> entry. We rarely, if ever want to swap the unnamed nodes.
>> 
>> Eli, does this require a NEWS entry or more documentation?
>
> IMHO a backend/helper function shouldn’t signal a user-error, it’s
> better to return nil and let the command to signal errors.

Yeah, I agree.  Adjusted the patch.  Let me know what you think!

Theo

[0001-Add-treesit-transpose-sexps-bug-60128.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 60128 <at> debbugs.gnu.org, casouri <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Tue, 27 Dec 2022 13:56:37 +0200

> From: Theodor Thornhill <theo <at> thornhill.no>
> Cc: casouri <at> gmail.com,  Stefan Monnier <monnier <at> iro.umontreal.ca>, eliz <at> gnu.org
> Date: Mon, 26 Dec 2022 21:53:57 +0100
> 
> Eli, does this require a NEWS entry or more documentation?

A NEWS entry should be enough.

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

From: Yuan Fu <casouri <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60128 <at> debbugs.gnu.org, Theodor Thornhill <theo <at> thornhill.no>,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Sat, 7 Jan 2023 15:18:17 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Theodor Thornhill <theo <at> thornhill.no>
>> Cc: casouri <at> gmail.com,  Stefan Monnier <monnier <at> iro.umontreal.ca>, eliz <at> gnu.org
>> Date: Mon, 26 Dec 2022 21:53:57 +0100
>> 
>> Eli, does this require a NEWS entry or more documentation?
>
> A NEWS entry should be enough.

IIRC this is pushed to master right? If so I’ll close this.

Yuan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Yuan Fu <casouri <at> gmail.com>, theo <at> thornhill.no
Cc: 60128 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Sun, 08 Jan 2023 13:56:33 +0200

> From: Yuan Fu <casouri <at> gmail.com>
> Date: Sat, 7 Jan 2023 15:18:17 -0800
> Cc: Theodor Thornhill <theo <at> thornhill.no>,
>  60128 <at> debbugs.gnu.org,
>  monnier <at> iro.umontreal.ca
> 
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Theodor Thornhill <theo <at> thornhill.no>
> >> Cc: casouri <at> gmail.com,  Stefan Monnier <monnier <at> iro.umontreal.ca>, eliz <at> gnu.org
> >> Date: Mon, 26 Dec 2022 21:53:57 +0100
> >> 
> >> Eli, does this require a NEWS entry or more documentation?
> >
> > A NEWS entry should be enough.
> 
> IIRC this is pushed to master right?

I don't see it there, no.

I asked to add a NEWS entry, and then it can be installed on master.

Theo?

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

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>, Yuan Fu <casouri <at> gmail.com>
Cc: 60128 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Sun, 08 Jan 2023 13:13:28 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Yuan Fu <casouri <at> gmail.com>
>> Date: Sat, 7 Jan 2023 15:18:17 -0800
>> Cc: Theodor Thornhill <theo <at> thornhill.no>,
>>  60128 <at> debbugs.gnu.org,
>>  monnier <at> iro.umontreal.ca
>> 
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> From: Theodor Thornhill <theo <at> thornhill.no>
>> >> Cc: casouri <at> gmail.com,  Stefan Monnier <monnier <at> iro.umontreal.ca>, eliz <at> gnu.org
>> >> Date: Mon, 26 Dec 2022 21:53:57 +0100
>> >> 
>> >> Eli, does this require a NEWS entry or more documentation?
>> >
>> > A NEWS entry should be enough.
>> 
>> IIRC this is pushed to master right?
>
> I don't see it there, no.
>
> I asked to add a NEWS entry, and then it can be installed on master.
>
> Theo?

Yeah, it was added in 7e98b8a0fa67f51784024fac3199d774dfa77192 and
commited by Stefan :-)

Should I add anything else?

Theo

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Yuan Fu <casouri <at> gmail.com>,
 60128-done <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#60128: 30.0.50; [PATCH]: Add treesit-transpose-sexps
Date: Tue, 5 Sep 2023 08:56:25 -0700

Theodor Thornhill <theo <at> thornhill.no> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Yuan Fu <casouri <at> gmail.com>
>>> Date: Sat, 7 Jan 2023 15:18:17 -0800
>>> Cc: Theodor Thornhill <theo <at> thornhill.no>,
>>>  60128 <at> debbugs.gnu.org,
>>>  monnier <at> iro.umontreal.ca
>>>
>>>
>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>>
>>> >> From: Theodor Thornhill <theo <at> thornhill.no>
>>> >> Cc: casouri <at> gmail.com,  Stefan Monnier <monnier <at> iro.umontreal.ca>, eliz <at> gnu.org
>>> >> Date: Mon, 26 Dec 2022 21:53:57 +0100
>>> >>
>>> >> Eli, does this require a NEWS entry or more documentation?
>>> >
>>> > A NEWS entry should be enough.
>>>
>>> IIRC this is pushed to master right?
>>
>> I don't see it there, no.
>>
>> I asked to add a NEWS entry, and then it can be installed on master.
>>
>> Theo?
>
> Yeah, it was added in 7e98b8a0fa67f51784024fac3199d774dfa77192 and
> commited by Stefan :-)
>
> Should I add anything else?
>
> Theo

It seems like this bug is done, as the patch was committed.  Closing.

Previous Next

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