refactor: remove nvim-treesitter as a dependency

[TheLeoP]

The plugin now uses a setup function to configure it and to initialize its user commands and autocmds.

Currently, most of the dependencies on nvim-treesitter where removed by copying the utility functions needed into this plugin, the plan would be to refactor again the code now that it is working without nvim-treesitter.

nvim-treesitter is only being used inside of scripts (check-queries, minimal_init and update_readme), is this ok? Should it be removed completely?

TODO:

• [x] refactor the code again (to not only used the copied functions from nvim-treesitter
• [x] check if autocmds on filetype are the best option
• [x] update README
• [x] update docs
• [ ] ~add some kind of on_attach like nvim-treesitter-context (?)~
• [x] refactor attach.lua
• [ ] should vim.treesitter.query.get be cached?
• [x] delete configuration fields for keymaps and let users define the keymaps themselves (?)
• [x] update tests
• [x] update scripts
• [x] add autoformat queries to CI (?)

This should address #503

[clason]

Thank you for tackling this! I think the best course here is to not merge this to master but make this a new parallel branch main (which will become the default branch when nvim-treesitter itself switches). The goal here is to make this work (only) in concert with the main branch of nvim-treesitter; mixing and matching main and master is not supported.

(This also means we don't have to complete the rewrite in a single PR.)

refactor the code again (to not only used the copied functions from nvim-treesitter)

💯 (and yes, this approach is what I would suggest as well)

check if autocmds on filetype are the best option

Probably not; I would enable this globally with a check for parsers and query that short-circuits them.

Also, it's perfectly fine (and often preferable) to have some global startup code in /plugin; the standard assumption is that the plugin should work without explicitly calling setup if you're fine with default options.

add some kind of on_attach like nvim-treesitter-context (?)

I consider nvim-treesitter-context to be the gold standard right now for a standalone treesitter plugin; I'd try to follow its lead as much as possible. (@lewis6991)

refactor attach.lua

Yes, attach/detach delendam esse. These are obsolete concepts. The assumption is that people enable these different functions themselves, however makes most sense for the functionality.

should vim.treesitter.query.get be cached?

delete configuration fields for keymaps and let users define the keymaps themselves (?)

Yes, definitely! We don't need yet another DSL for setting keymaps. (Although we could talk about reasonable default mappings as opt-in.)

Although possibly this is a special case here; maybe we could discuss instead exposing a single (Lua!) function that is easy to map?

nvim-treesitter is only being used inside of scripts (check-queries, minimal_init and update_readme), is this ok?

That is OK; you need nvim-treesitter (main!) to set up the test environment.

[lewis6991]

should vim.treesitter.query.get be cached?

Correct me if I'm wrong, but doesn't core already cache this via a weak table?

[theHamsta]

should vim.treesitter.query.get be cached?

Correct me if I'm wrong, but doesn't core already cache this via a weak table?

I believe you're right, they should be cached. Nvim-treesitter-textobjects might be doing additional caching of the query results if they are using still an own slow algorithm to aggregate query results

[clason]

Are there any plans to keep the commands? We've added them because some users requested them because they wanted to use them in ext mode or for vinyl remappings. Not a strong requirement since all functionality is available via Lua functions, just being curious on what your idea for this PR was.

I think we should take this opportunity to revisit the user API here, with all the lessons learned since. (Not necessarily in this PR.)

My preference would be to start with a clean slate and (re-)add stuff when it is requested with a compelling use case. (User commands are trivial to add in a plugin file, as we do in nvim-treesitter now.)

What I don't want to see again is a custom module for creating keymaps and user commands through setup().

[TheLeoP]

Thank you for tackling this! I think the best course here is to not merge this to master but make this a new parallel branch main (which will become the default branch when nvim-treesitter itself switches).

How should I tackle this? I see that I can change which branch the PR targets, but I can't select the main branch since it does not exit yet. @clason

[clason]

Now it does :)

[lucario387]

So make-range was never needed? 😱

[clason]

should vim.treesitter.query.get be cached?

Correct me if I'm wrong, but doesn't core already cache this via a weak table?

Does this mean the memoize in nvim-treesitter (indent, locals) is obsolete now?

[lucario387]

Looks like that

[clason]

Nice; want to make a PR to remove it from the main branch? (After local testing, which -- at least for indents -- you're in the best position to do.)

[lucario387]

Sadly, my previous comment was incorrect.

vim.treesitter.query.{get,parse} is cached, but the result of it is used for memoize of locals of indents, so neovim core's caching is for the parsing, while nvim-treesitter's caching is for the handling of such parse

[clason]

So we will need the same sort of memoization here for the textobjects query results, right?

[lucario387]

So we will need the same sort of memoization here for the textobjects query results, right?

Would be nice, I don't think there's one right now though

[TheLeoP]

So make-range was never needed? 😱

I didn't understand the codebase enought to know what make-range does, so I deleted it. I just added it again (because I couldn't make the test pass because parameter.outer in Python uses make-range), sorry :c.

[lucario387]

@TheLeoP basically we create a new node capture from the two node captures that is specified for make-range,

[TheLeoP]

@clason Is there a specific part of the code left that you would like to be refactored? I still haven't give too much atention to test, docs or scripts, I wanted to focus on the plugin functionality first.

[clason]

@clason Is there a specific part of the code left that you would like to be refactored? I still haven't give too much atention to test, docs or scripts, I wanted to focus on the plugin functionality first.

I think you are right that we should first make sure everything works with nvim-treesitter main and that all tests pass; afterwards it's much easier to make changes one by one (e.g., going through the backported utility functions to see if they can be replaced by Neovim core APIs).

Also, we have an autoformatter for queries now, which could be added to CI here ;)

[clason]

No, this comment stands. I want to see the module framework gone. If you want to set keymaps (through this plugin), use the Neovim API directly.

[ObserverOfTime]

Yes, I didn't mean it should remain a module.

[clason]

@TheLeoP this looks like it's in a good place -- can you take care of the remaining items?

[TheLeoP]

@clason. Will do. Question: are the current vimdocs generated? Or do they have to be manually updated?

[clason]

They are manually written. It's OK to have a minimal (but correct) version for the initial PR.

[TheLeoP]

Is it expected to document the lua API being used for keymaps on the vimdocs/README?

[clason]

It's a matter of taste whether you consider the help file or the README as the primary documentation. It certainly doesn't hurt to have it in the README (and shows that we really need to rework the interface here... follow-up PR, though).

[mehalter]

Hey @TheLeoP this is an amazing PR and is looking great! I pulled it up and started testing it out and I noticed one weirdness so far when I do something like a selection in a filetype that doesn't support a query. Say I do like vic that I configured for visually selecting within a class in a filetype that doesn't support the @class query like Lua it gives me the notification that the filetype is not supported but it leaves me in visual mode which I don't expect. Could we have it on a failed attempt to also put it back in normal mode or something? I'm not sure if that makes sense.

Another thing is the notification you get is "This filetype is not supported by nvim-treesitter-textobjects" which is a bit misleading. It would be nice to give a more contextual warning like "The lua parser does not support the @class query" or something like that.

[clason]

To be clear, these are regressions from the current master branch? Otherwise this is totally out of scope of this PR.

[mehalter]

They are sort of regressions from the current master. The way the current master works is it only creates mappings that are supported so you don't run into this case of a binding that isn't supported. The addition of the error message I mentioned is in this pull request.

[ObserverOfTime]

The way the current master works is it only creates mappings that are supported so you don't run into this case of a binding that isn't supported.

Proof that we need to keep the keymap config. ;)

[clason]

Yeah, yeah. (But not the old system, that needs to die in a fire.)

[TheLeoP]

Currently, the approach is to check (by using require'nvim-treesitter-textobjects.shared'.check_support) whether a filetype, query file, and capture are supported every time a function intended for mapping (i.e. require "nvim-treesitter-textobjects.select".select_textobject("@function.outer", "textobjects", mode)) is called, so the user has to create only one keymap.

Another option could be to instructs users into creating keymaps inside a filetype autocmd that uses check_support before creating a buffer scoped keymap. This would be similar to how LSP keymaps are usually created inside an LspAttach autocmd and would be consisten with the current behavior.