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

Simplify first-in-line computation for indent queries #10527

Merged

Conversation

Triton171
Copy link
Contributor

I originally thought that it might be necessary to look at the syntax tree to decide whether a node is the first node on its line. Since then, I haven't really seen any example where this is the case, so we can replace my initial implementation with a simpler one that just checks if all preceeding characters on the same line are whitespace.

This fixes #10336 and should generally prevent issues with tree-sitter grammars that insert nodes for whitespace.

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-indent Area: Indentation labels Apr 20, 2024
the-mikedavis
the-mikedavis previously approved these changes Apr 20, 2024
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up! The new implementation makes sense to me. But I am curious what cases the old one was supposed to cover that the simplified one doesn't

@@ -863,7 +839,7 @@ pub fn treesitter_indent_for_pos<'a>(
loop {
// This can safely be unwrapped because `first_in_line` contains
// one entry for each ancestor of the node (which is what we iterate over)
Copy link
Member

Choose a reason for hiding this comment

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

This comment can be removed now

This fixes issues for grammars with nodes containing only whitespace (e.g. Go).
@Triton171
Copy link
Contributor Author

But I am curious what cases the old one was supposed to cover that the simplified one doesn't

It was supposed to correctly handle things like lines starting with a specific symbol that isn't considered a node in the syntax tree, e.g. for marking an environment. The main example I could think of is Literate Haskell, where lines starting with > are code and everything else is a comment (but the > character shouldn't really appear in the syntax tree). In hindsight, this is probably too much of a niche feature to be worth the effort though.

@the-mikedavis the-mikedavis merged commit efae85e into helix-editor:master Apr 20, 2024
6 checks passed
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-indent Area: Indentation S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hybrid indentation can cause incorrect results at the end of indent scopes
3 participants