Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(motion): allows movement to parent form's edges using (, ) #38

Merged
merged 4 commits into from
Oct 2, 2023

Conversation

Samy-33
Copy link
Contributor

@Samy-33 Samy-33 commented Sep 30, 2023

closes #34

@Samy-33 Samy-33 changed the title feat(motion): allows to move to parent forms edges using (, ) feat(motion): allows movement to parent form's edges using (, ) Sep 30, 2023
@armed
Copy link
Collaborator

armed commented Oct 1, 2023

Hey @Samy-33, thanks. Looks sick. Can you also update the readme with new keymaps?

@Samy-33
Copy link
Contributor Author

Samy-33 commented Oct 1, 2023

@armed, thanks for taking a look. Done.

@julienvincent
Copy link
Owner

julienvincent commented Oct 1, 2023

Thanks for taking the time to add this! Everything looks good code-wise.

I'm just unsure about the behaviour of moving to the parent from a form edge.

In this implementation it moves to the nodes direct parent and not the parent form:

(1 @|(a))
=>
(1 |@(a))

I feel it makes more sense to move to the next parent form bracket.

(1 @|(a))
=>
|(1 @(a))

@Samy-33
Copy link
Contributor Author

Samy-33 commented Oct 1, 2023

I was also wondering if the motion should only be to brackets and was surprised when Treesitter would say that @(...) is a form when the cursor is on @. And rightly so. @ is a short form for deref. Which would have been visibly a form (deref |(...)) and would have moved to |(deref (...))

This behaviour only happens for shortcuts for a form, and in my opinion, it makes more sense to keep consider @ as well when moving to form's head.

As af, if also consider deref (@) a form. This also makes it consistent with the grammar. Let me know what you think.

@julienvincent
Copy link
Owner

julienvincent commented Oct 1, 2023

I was also wondering if the motion should only be to brackets and was surprised when Treesitter would say that @(...) is a form when the cursor is on @. And rightly so. @ is a short form for deref. Which would have been visibly a form (deref |(...)) and would have moved to |(deref (...))

This behaviour only happens for shortcuts for a form, and in my opinion, it makes more sense to keep consider @ as well when moving to form's head.

This isn't quite accurate - Treesitter knowns nothing about Clojure and is not in itself considering the @ binding to be a form, just a parent in the AST. A form is a Clojure specific concept.

The way the Clojure TS grammar is structured the @ node is the forms parent, just as the # in a Clojure set (#{}) is structurally the parent of the TS node {}. This doesn't mean we consider the # component of a Clojure set a form.

The same is true for other grammar components such as:

`()
#{}
#_()
`#{}

etc.

I don't think in any of these cases it makes sense to treat the prefixes to these forms as forms themselves and we should instead be jumping to the nearest parent form (not just the treesitter AST parent).

As af, if also consider deref (@) a form. This also makes it consistent with the grammar. Let me know what you think.

These bindings don't exactly consider form prefixes as a form itself, but when using af it generally makes sense to operate on the entire root node including it's suffixes as not doing so would usually not be syntactically correct (for example, performing a daf on #(1)).

I do not think the same applies for navigating between brackets.


I think the expected behaviour is more likely to be that these navigation functions move between form brackets only, ignoring node markers.

@julienvincent
Copy link
Owner

julienvincent commented Oct 2, 2023

FWIW I gave vim-sexp(-mappings-for-regular-people) a quick test to compare how it behaves and they also jump only between brackets, ignoring form markers like @. I don't think there is a good reason not to do the same.

All we really need to change to support this is to wrap

form_node_to_move_to = form_node_to_move_to:parent()

in lang.get_node_root. Like so:

form_node_to_move_to = lang.get_node_root(form_node_to_move_to):parent()

@Samy-33
Copy link
Contributor Author

Samy-33 commented Oct 2, 2023

You're right @julienvincent. I assumed that treesitter considered @ as a form node. Apparently that was not the case.

Thanks for pointing it out.

Copy link
Owner

@julienvincent julienvincent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool this all looks good! Thanks for the contribution!

@julienvincent julienvincent merged commit 2f117b3 into julienvincent:master Oct 2, 2023
1 check passed
@armed
Copy link
Collaborator

armed commented Oct 2, 2023

Yep, really useful addition, actually I'm still using vim-sexp with some mappings partially enabled. Parens one of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: Move to Next/Prev bracket
3 participants