-
Notifications
You must be signed in to change notification settings - Fork 2
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
lets rename to idris2? #5
Comments
except, I wonder if https://github.com/idris-community/idris2-lsp/blob/81e70d48b7428034b8bc1fa679838532232b5387/src/Server/ProcessMessage.idr#L396 will continue working @clason maybe You have an insight? is it using |
You are missing the point here. If the language name is |
ok here is my proposal
https://github.com/AstroNvim/astrocommunity/pull/1258/files reasonmaybe there will be idris 3 - https://discord.com/channels/827106007712661524/827106088343175180/1271141058289598619 proof that everything works(in bottom highlight is different bc from lsp) |
@clason why thumb down? |
Because there's a lot of noise here about nothing. As long as Idris2 and Idris (and the -- so far -- mythical Idris3) are not completely incompatible languages, there's no point in renaming the grammar. (You don't have different parsers for Python 2 and Python 3, either.) And, again, Vim filetypes are a completely different matter and can be handled on the Neovim side just fine, without needing any change here (which will affect every other consumer). |
I agree Should I then propose to change https://github.com/neovim/nvim-lspconfig/blob/bc6ada4b0892b7f10852c0b8ca7209fd39a6d754/lua/lspconfig/configs/idris2_lsp.lua#L6 with 'idris' instead of 'idris2'? |
No, why? As long as there's no official filetype, all this is just pointless busywork. (And that project has nothing to do with tree-sitter -- or me.) |
@clason so, You propose
P.S. I propose to at least rename this repo from https://github.com/kayhide/tree-sitter-idris to https://github.com/kayhide/tree-sitter-idris2 because it doesnt support idris1 @kayhide P.P.S. will ask in discord to provide more input from community |
If I can add my 2c, after having seen 450+ tree sitter projects (while working on https://github.com/alexaandru/go-sitter-forest): I haven't seen this pattern in any other parser out there. All languages have versions, and many have multiple versions, but none of them have the grammar name also include the version. |
🤓 Well, there is https://github.com/Joakker/tree-sitter-json5 (and https://github.com/dbt-labs/tree-sitter-jinja2). For the record, there already is an idris2 parser: https://github.com/gwerbin/tree-sitter-idris2 (archived, though). |
But since the OP and the comments are a bit all over the place, let me summarize the situation from the Neovim side:
So in the end, it boils down what makes most sense for the language community. In my view, a different name is only necessary if you need or want to have different parsers for different language versions -- which doesn't seem like it's the case. The only aspect might be discoverability: Having it named "idris2" would be a clear signal that the parser supports Idris2, possibly preventing questions about it. Whether that's worth the hassle of going through a full rename (potentially breaking existing consumers) is another question. |
Umm, json5 is a really poor example ;-) since the 5 is an integral part of the language name, json5 is at version 2 right now (https://github.com/json5/json5/tree/v2.2.3), but you don't see the grammar name as json52, it is still |
Yeah, that was tongue in cheek, in case it wasn't clear. Anyway, my point was that there's precedent (and small convenience for Vimland) in naming the parser |
I dont like this too, I wish it was called just idris, but
|
@kayhide Since vim/vim#15987 was merged and https://github.com/kayhide/tree-sitter-idris doesnt support idris2, and then I will make pr to rename and we can close this issue |
Thank you for the discussion, but I am on the side of NOT changing the name as @srghma suggested. And there may be one misundersting, let me clarify. Aside the discussion above, it seems twisted to make such a change on parser's side for the sake of editor's convenience. |
....css vendor prefixes I was thinking about names too @kayhide do You think I should make pr and change https://github.com/vim/vim/pull/15987/files to
? I think yes, bc lean4 is just lean |
No, you should not make such a change! I am frankly getting annoyed at all these pointless "let's change this!" comments. Things are fine exactly how they are now. |
@srghma Well, if you have a good reason for that change. |
to follow up #1
the nvim-lspconfig will only work if current lifetype is idris2 https://github.com/neovim/nvim-lspconfig/blob/54617a18f4cf46f0c2f6d024fa6feb7515fe036d/lua/lspconfig/configs/idris2_lsp.lua#L6
but treesitter will throw if I name it idris2 (it creates filetype I think) nvim-treesitter/nvim-treesitter#7274 (comment) (because in grammar.js name is
idris
, but and this name should be the same aslist.NAME
)and then I map filetype of file here https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840R40
and to make lsp work I make it boot on idris filetype too https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840R64
THEREFORE:
we can either
name
in grammar.js)filetypes = { 'idris2' },
tofiletypes = { 'idris' },
and then delete https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840R64I think better to 1. rename this repo, bc https://github.com/neovim/nvim-lspconfig/blob/54617a18f4cf46f0c2f6d024fa6feb7515fe036d/lua/lspconfig/configs/idris2_lsp.lua#L6 was first to define it as
idris2
P.S. and then maybe as Helix to change language from idris to idris2 https://github.com/helix-editor/helix/blob/38faf74febf3332fb119302324bfd21229d39e14/languages.toml#L2350 (idris has keywork
postulate
, idris2 - doesnt, it uses%unsafe\nfoo : MyType
instead)The text was updated successfully, but these errors were encountered: