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

Update just grammar and queries #9850

Closed
wants to merge 1 commit into from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Mar 11, 2024

Bump the git revision and use the queries from https://github.com/IndianBoy42/tree-sitter-just/tree/80d612d198b8d6207416225c9fcea0fe86ffb96e/queries-flavored/helix to update Helix's sources.

Tree-sitter-just has queries for Helix in-tree, I will make any changes from this review upstream.

Originally added at #6453 (cc @VuiMuich)

Fixes: #8226

@woojiq
Copy link
Contributor

woojiq commented Mar 12, 2024

I don't have a lot of Justfiles, but judging by a quick review, I can tell that it is much better than on master. I also appreciate the comments in queries which help to understand them faster.
I once wanted to bump the version of tree-sitter-just because there were no working injection queries. Now something like this works fine: set shell := ["fish", "-c"], but I still wonder if it is possible to support injections inside a recipe:

python:
  #!/usr/bin/env python3
  print('Hello from python!')

js:
  #!/usr/bin/env node
  console.log('Greetings from JavaScript!')

perl:
  #!/usr/bin/env perl
  print "Larry Wall says Hi!\n";

@the-mikedavis
Copy link
Member

I believe those injections should work but currently the pattern that defaults other recipe_bodys to bash is taking precedence no matter which order the patterns are defined in. (I think because the shebang pattern is more specific, so it matches last.) It might work to add a field for shebang so you could match against the absence of the field:

(recipe_body
  !shebang
  (#set! injection.language "bash")
  (#set! injection.include-children)) @injection.content
(recipe_body
  shebang: (shebang) @injection.shebang
  (#set! injection.include-children)) @injection.content

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-language-support Area: Support for programming/text languages labels Mar 12, 2024
@tgross35
Copy link
Contributor Author

I believe those injections should work but currently the pattern that defaults other recipe_bodys to bash is taking precedence no matter which order the patterns are defined in. (I think because the shebang pattern is more specific, so it matches last.) It might work to add a field for shebang so you could match against the absence of the field:

(recipe_body
  !shebang
  (#set! injection.language "bash")
  (#set! injection.include-children)) @injection.content
(recipe_body
  shebang: (shebang) @injection.shebang
  (#set! injection.include-children)) @injection.content

Thanks, I did mean to ask about this specifically since it seemed like order in the query file did not matter. I'll play with this a bit - is the ! syntax standard? I don't see it documented on the tree-sitter website.

@tgross35
Copy link
Contributor Author

Another question, is there any good way to test indents, injections, and textobjects queries?

@the-mikedavis
Copy link
Member

It's standard, see "negated fields" here under the docs on the query format: https://tree-sitter.github.io/tree-sitter/using-parsers#query-syntax. It's a small section that's easy to miss.

Currently we don't have unit tests for queries. We could add something like the query testing in tree-sitter-cli to the xtask crate but it would add a lot of code for something that is usually not too much work to verify by hand

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 14, 2024

Using !shebang turned out pretty great based on tests from IndianBoy42/tree-sitter-just#151. Default to bash:

image

Specifying via shebang:

image

Setting globally:

image

But Helix 32.10 doesn't seem to pick up the shebang #!/bin/sh for some reason. I don't know why this would be since it is covered in https://github.com/helix-editor/helix/blame/6c4d986c1b1ac4e350dced513b6608ba4464cde3/languages.toml#L911 and picked up by the regex in

const SHEBANG: &str = r"#!\s*(?:\S*[/\\](?:env\s+(?:\-\S+\s+)*)?)?([^\s\.\d]+)";

image

It doesn't pick up #!/bin/dash either, but does pick up #!/bin/bash.

image

@RoloEdits
Copy link
Contributor

Has this been confirmed to fix #8226 ?

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 15, 2024

Has this been confirmed to fix #8226 ?

I believe so from a quick test, added a Fixes

@the-mikedavis
Copy link
Member

Could you push the latest changes to this branch so I could debug? I see the #!/bin/sh shebang working on regular files (for example starting hx file on a file that only contains the shebang) so I don't see why it wouldn't work in an injection

@tgross35
Copy link
Contributor Author

Could you push the latest changes to this branch so I could debug? I see the #!/bin/sh shebang working on regular files (for example starting hx file on a file that only contains the shebang) so I don't see why it wouldn't work in an injection

Sorry @the-mikedavis , this slipped my mind. They are updated now

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 25, 2024
@the-mikedavis
Copy link
Member

Looks like the issue is that we look up languages that are tagged with @injection.shebang by looking only at language IDs (names):

helix/helix-core/src/syntax.rs

Lines 1024 to 1026 in 50c90cb

InjectionLanguageMarker::Shebang(shebang) => {
self.language_config_for_language_id(shebang)
}

We should look up in language_config_ids_by_shebang instead like here:

.and_then(|cap| self.language_config_ids_by_shebang.get(&cap[1]));
configuration_id.and_then(|&id| self.language_configs.get(id).cloned())

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 24, 2024
@RoloEdits
Copy link
Contributor

@tgross35 Where does this pr's status stand? Are the queries all done, just needing the language_config_ids_by_shebang fix?

@tgross35
Copy link
Contributor Author

Ah I'm sorry, I just never got back to this. Yeah, to my knowledge it only needs that fix. I haven't looked into it so please feel free to pick that part up (or even this whole PR if you would prefer).

@RoloEdits
Copy link
Contributor

with #11306 merged this can now be closed

@tgross35
Copy link
Contributor Author

Oh awesome, thank you for getting this over the line!

@tgross35 tgross35 closed this Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

newline at eof in justfile creates livelock
4 participants