-
-
Notifications
You must be signed in to change notification settings - Fork 84
Introduce tree-sitter queries for syntactic scopes #629
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
Conversation
Shouldn't we use https://github.com/nvim-treesitter/tree-sitter-query instead of scheme? |
Also worth mentioning https://marketplace.visualstudio.com/items?itemName=jrieken.vscode-tree-sitter-query, which defines a language id for tree sitter queries |
sg! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just publishing a few random comments here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice progress 😊. Definitely pretty clean / readable syntax. Left a few nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't read whole PR yet but here's some comments
As discussed in meet-up, algorithm is as follows:
|
772442c
to
f775401
Compare
f775401
to
ecaf27f
Compare
Ok this one is good to go, but unfortunately it breaks "every funk" for Ruby. I created #1430 to fix that issue. That PR is ready, so would be great if you could have a look when you get a minute. I think they should prob merge in together so we don't break "every funk" in Ruby on main |
- Adds `@_.domain`, `@_.leading`, `@_.trailing`, `@_.removal`, and `@_.interior` tree-sitter query captures - Introduces `@_.iteration` tree-sitter query capture to allow indicating iteration scope for tree-sitter scope handlers - Depends on #629 ## Checklist - [ ] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [ ] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [ ] I have not broken the cheatsheet
) This PR adds a test to ensure that we don't try to define the same scope type both using our legacy node matchers and the new query scope handlers I have mixed feelings about the way I made this work, but I wasn't sure how else to do it. The test needs to be defined in the engine, because otherwise we'd need to expose lots of stuff to cursorless-vscode-e2e, but it needs a full tree-sitter instance, so can't be run as a unit test. This file is the compromise I came up with 🤷♂️. We should maybe revise once we have tree-sitter integrated with this repo I don't like that this code gets rolled up into our production bundle, but probably ok for this one-off. If we find this file growing, we should revisit It's also possible that once we get rid of our `ide` singleton, we can relax the restriction that our test harness doesn't import from `cursorless-engine`. That would enable us to just import what we need from the engine directly in the test harness to run this test 🤔 - Depends on #629 ## Checklist - [ ] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [ ] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [ ] I have not broken the cheatsheet
Todo
iterationScope
? That's the only thing that is a regression here.