-
Notifications
You must be signed in to change notification settings - Fork 33
refactor!: cleanup rules that can be assimilated as commands #220
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
Big savings. I was hoping one day to see if the tree-sitter-nu parser would be faster than the nushell parser. However, if there's no hope for ever using it for anything other than and in-editor parser, then it's probably good to optimize it for that. |
@fdncred yeah, I understand the intention. However according to what I learned from the tree-sitter community, it's generally not recommended to use a tree-sitter parser in a compiler/interpreter. Besides, performance-wise, I believe the new-nu-parser(current nu-parser can be slow) should be by no means slower than TS. The only thing missing is the incremental nature. |
(ctrl_return) @statement.outer @return.outer | ||
(ctrl_do) @statement.outer | ||
|
||
(ctrl_if) @statement.outer | ||
|
||
(ctrl_try) @statement.outer | ||
|
||
(ctrl_match) @statement.outer | ||
|
||
(ctrl_while) @statement.outer | ||
|
||
(ctrl_loop) @statement.outer | ||
("break" "continue") @statement.outer |
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.
@Bahex I've merged ctrl_do
/ctrl_return
/"break"/"continue" to our command
rule.
I'm not sure whether some features will be missing if we remove these queries, do you have any idea?
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.
It should be fine as far as I can tell, and even if we want to add queries for these in the future, we can just write the them for command
nodes with a specific head.
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.
@Bahex Would you like to review/test this PR since there are some breaking changes?
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.
nvim-treesitter-context
has a query that would need to be updated, but other than that I haven't found anything that breaks.
Thanks for the reminder, I haven't updated the parser-rev/query-files of nvim-treesitter since cli 0.25+ was applied in this repo. |
Should we go ahead and land this @blindFS? |
I think so. Given there're already big gaps between the main branch and those locked by editors/plugins, it won't break things further. |
Thanks |
The idea of #179
It helps with the WASM compilation time #185 : case count 3146 -> 2592, time 2m20 -> 1m35
IMHO the target of tree-sitter-nu is only for code text manipulation, it's never gonna be as complete/semantically accurate as the native parser. And those rules are unnecessary in that regards.