Skip to content

feat: add Syntax::layers_for_byte_range #9

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nik-rev
Copy link

@nik-rev nik-rev commented May 14, 2025

When I tried to migrate the comment continuation PR (helix-editor/helix#12759) to use the new tree-house bindings, I encountered a problem:

When continuing comments, we look at the smallest injection layer that includes the selection. This layer is the comment language itself, which has no tokens. We need to ignore it and try the next layer. But there's no way to do that currently

I think returning a list of layers from largest to smallest that include the byte range is a good API for tree-house

I updated my helix PR to make use of this PR: https://github.com/helix-editor/helix/blob/f07e6973fedb2fed4ff7f0e1a9d166bea7bedcab/helix-core/src/comment.rs#L58-L77

Public API

impl Syntax {
    /// # Returns
    ///
    /// A list of layers which **fully include** the byte range `start..=end`,
    /// in decreasing order based on the size of each layer.
    ///
    /// The first layer is the `root` layer.
    pub fn layers_for_byte_range(&self, start: u32, end: u32) -> Iterator<Item = Layer>;
}

@the-mikedavis
Copy link
Member

We should avoid the Vec if we can and have this be an iterator instead.

Alternatively we could expose parent on LayerData. You could use that with the existing layer_for_byte_range to find all layers.

@nik-rev
Copy link
Author

nik-rev commented May 15, 2025

We should avoid the Vec if we can and have this be an iterator instead.

Alternatively we could expose parent on LayerData. You could use that with the existing layer_for_byte_range to find all layers.

I assume you'd like to avoid the Vec to not have to make extra allocations?
In any case I updated the PR to use iter::from_fn so it returns Iterator<Item = Layer> now

@nik-rev
Copy link
Author

nik-rev commented May 15, 2025

Also I'm curious why did you choose to receive start: u32, end: u32 rather than range: impl RangeBounds<u32>?

@nik-rev nik-rev marked this pull request as draft May 16, 2025 20:34
@nik-rev nik-rev marked this pull request as ready for review May 16, 2025 20:43
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.

2 participants