-
-
Notifications
You must be signed in to change notification settings - Fork 140
Avoid pcall on vim.treesitter.language.add as it doesn't assert/error #750
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
| (let [add (case vim.treesitter | ||
| {:language {:add f}} f | ||
| {:language {:require_language f}} (partial pcall f) | ||
| {:require_language f} (partial pcall f))] |
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 doesn't appear that the clause on L.196 would ever match because :require_language isn't a key in vim.treesitter.
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 no longer exists in more current versions of Neovim, but I preserved this case since the old version of the Conjure code was falling back to it too. Here's the 2020 Neovim commit where it was introduced and here's the (also 2020) commit where it was moved from top-level vim.treesitter to be under vim.treesitter.language. For completeness' sake, here's the 2024 commit where it was renamed from require_language to add.
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, I would also point out that our minimum version is v0.10 so we should try to be backwards compatible to there but if someone's on <0.10 Conjure will error anyway. I didn't look into the version numbers these changes correspond to but I'm okay with this backwards compatibility regardless, just thought I'd mention the version threshold anyway.
I also didn't know Fennel case statements could be used like this, I really like it!
|
Good catch! Your changes make it clear what a user needs to do if they don't have a Tree-sitter parser for Python. Without your changes, the user has to suffer these error messages:
|
| (let [add (case vim.treesitter | ||
| {:language {:add f}} f | ||
| {:language {:require_language f}} (partial pcall f) | ||
| {:require_language f} (partial pcall f))] |
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, I would also point out that our minimum version is v0.10 so we should try to be backwards compatible to there but if someone's on <0.10 Conjure will error anyway. I didn't look into the version numbers these changes correspond to but I'm okay with this backwards compatibility regardless, just thought I'd mention the version threshold anyway.
I also didn't know Fennel case statements could be used like this, I really like it!
pcall-ingvim.treesitter.language.addwould always return true as its first result value, asadduses the style of returning nil and an error message when it can't add the language rather than aborting viaerrororassert. This causes an issue when a Conjure user opens a Python buffer but doesn't have a Python treesitter parser installed. Conjure thinks they do have a parser installed and tries to start the stdio client. The user would see an error from Neovim's treesitter code likevim.schedule: No parser for language "python", and not the stdio client's intended log messages for this case (reproduced below) which made it difficult to identify that the error was originating from Conjure.The style of return nil plus error message is a change from the now-deprecated
require_languagefunctions that Conjure'sadd-languagealso wraps for backwards-compatiblity. Thepcallwas previously around theadd-languagewrapper call in the Python stdio client'sstartfunction. These changes move the choice topcallor not into theadd-languagewrapper itself. This letsaddskip apcallwhile the deprecatedrequire_languagefunctions still get apcall.