Add align to ancestor node type line indentation functionality #12

Merged
FelipeLema merged 7 commits from joejunior/tree-sitter-indent.el:main into main 9 months ago

While trying to add C# support (in the csharp-mode package) I noticed that the following common behavior was not possible:

void Bar()
{
    Foo(() =>
    {
        System
            .Console
            .Write("Bar");
    });

    OtherFoo(param1, 
        param2,
        () =>
        {
            Bib();
        });
}

If argument_list node type or argument node type is set to always-indent, the block opener would always be indented, as well.

This pull adds align-to-node-line, a group that allows to set a list of node types that the current node will try to align to its indentation, instead of following its parent indentation. The above example has the following configuration to work (ommiting the other indentation rules, I'm only focusing on the relevant ones):

(indent-all . ;; these nodes are always indented
                (argument_list))
(align-to-node-line . ;; these nodes are aligned to the first column of the
                      ;; line where the first node contained in the list is found.
                    ((block . (lambda_expression)))))

So, instead of the block inheriting the indentation of argument_list it tries to find an ancestor with type lambda_expression, finding one, it indents to its line's first column. If no lambda_expression is found, the other rules follows as normal.

While trying to add C# support (in the csharp-mode package) I noticed that the following common behavior was not possible: ``` void Bar() { Foo(() => { System .Console .Write("Bar"); }); OtherFoo(param1, param2, () => { Bib(); }); } ``` If `argument_list` node type or `argument` node type is set to `always-indent`, the block opener would always be indented, as well. This pull adds `align-to-node-line`, a group that allows to set a list of node types that the current node will try to align to its indentation, instead of following its parent indentation. The above example has the following configuration to work (ommiting the other indentation rules, I'm only focusing on the relevant ones): ``` (indent-all . ;; these nodes are always indented (argument_list)) (align-to-node-line . ;; these nodes are aligned to the first column of the ;; line where the first node contained in the list is found. ((block . (lambda_expression))))) ``` So, instead of the block inheriting the indentation of `argument_list` it tries to find an ancestor with type `lambda_expression`, finding one, it indents to its line's first column. If no `lambda_expression` is found, the other rules follows as normal.
joejunior added 2 commits 9 months ago
FelipeLema requested changes 9 months ago
(let ((scope (alist-get (tsc-node-type current-node) align-to-node-line-alist)))
(when scope
(let* ((reverse-path (reverse parentwise-path))
(ancestors-path (nthcdr (+ 1 (cl-position current-node reverse-path)) reverse-path))
Poster
Owner

This line looks un-aligned. Can you please correct it?

This line looks un-aligned. Can you please correct it?
FelipeLema marked this conversation as resolved
(lambda (ancestor-node)
(member (tsc-node-type ancestor-node) scope))
ancestors-path)))
(when align-to-node
Poster
Owner

you can also use when-let* and spare this when clause

you can also use `when-let*` and spare this `when` clause
(member (tsc-node-type ancestor-node) scope))
ancestors-path)))
(when align-to-node
(save-excursion
Poster
Owner

Can you state your goal before save-excursion and forward-line-ing?
I know it's redundant… but when documenting code, redundancy can be handy

Can you state your goal before `save-excursion` and `forward-line`-ing? I know it's redundant… but when documenting code, redundancy can be handy
Poster

I tried to simplify and explain better. Do you think it needs futher explanation?

I tried to simplify and explain better. Do you think it needs futher explanation?
FelipeLema marked this conversation as resolved
FelipeLema reviewed 9 months ago
(- first-sibling-position
(line-beginning-position)))))))
(defun tree-sitter-indent--align-node-line-column
Poster
Owner

Can you add the sample code you provided as unit test for this feature?

(of course, run make test to make sure it's working all right)

Can you add the sample code you provided as unit test for this feature? (of course, run `make test` to make sure it's working all right)
Poster

I had to use csharp-mode with the experimental tree-sitter support. The tree-sitter pattern and indentation rules are there.

I don't know if it makes much sense to put this test, unless C# support is added by default, or we add csharp-mode as test dependency.

I had to use csharp-mode with the experimental tree-sitter support. The tree-sitter pattern and indentation rules are there. I don't know if it makes much sense to put this test, unless C# support is added by default, or we add csharp-mode as test dependency.
Poster
Owner

that's ok to add csharp-mode as a dependency. We're already doing this with rust-mode at the bottom of setup-tests.el

you can go ahead and add (straight-use-package 'csharp-mode) there and then add the unit test

that's ok to add csharp-mode as a dependency. We're already doing this with rust-mode at [the bottom of setup-tests.el](https://codeberg.org/FelipeLema/tree-sitter-indent.el/src/branch/main/setup-tests.el#L28) you can go ahead and add `(straight-use-package 'csharp-mode)` there and then add the unit test
Owner

thank you for taking the time to improve this package.

I left some comments for you to address (we want to keep this code easy to maintain). If you need help with any, let me know and I'll give you a hand

thank you for taking the time to improve this package. I left some comments for you to address (we want to keep this code easy to maintain). If you need help with any, let me know and I'll give you a hand
joejunior added 2 commits 9 months ago
joejunior requested review from FelipeLema 9 months ago
FelipeLema requested changes 9 months ago
FelipeLema left a comment

thanks for addressing the comments... I'm returning this review so we can work together on adding the sample code as unit test (see my instructions in comment)

joejunior added 1 commit 9 months ago
joejunior requested review from FelipeLema 9 months ago
Poster

I added the test. However, since I'm working on adding this feature to csharp-mode, I had to use my fork as a requirement for the test. When the PR there is accepted, we can update to use the main.

I added the test. However, since I'm working on adding this feature to csharp-mode, I had to use my fork as a requirement for the test. When the PR there is accepted, we can update to use the main.
joejunior added 1 commit 9 months ago
e132452d95 Add fix in align-to-node-line group that makes it not align to line
joejunior added 1 commit 9 months ago
FelipeLema approved these changes 9 months ago
FelipeLema left a comment

looks good. thanks for doing the changes in such short time

FelipeLema merged commit 18d263720c into main 9 months ago

Reviewers

FelipeLema approved these changes 9 months ago
The pull request has been merged as 18d263720c.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.