Skip to content
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

Closed
srghma opened this issue Nov 1, 2024 · 20 comments
Closed

lets rename to idris2? #5

srghma opened this issue Nov 1, 2024 · 20 comments

Comments

@srghma
Copy link
Collaborator

srghma commented Nov 1, 2024

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 as list.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

  1. rename this repo from https://github.com/kayhide/tree-sitter-idris to https://github.com/kayhide/tree-sitter-idris2 (most importantly - the name in grammar.js)
  2. or can make pr to https://github.com/neovim/nvim-lspconfig/blob/54617a18f4cf46f0c2f6d024fa6feb7515fe036d/lua/lspconfig/configs/idris2_lsp.lua#L6 and change filetypes = { 'idris2' }, to filetypes = { 'idris' }, and then delete https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840R64
  3. leave https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840R64 as is

I 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)

@srghma
Copy link
Collaborator Author

srghma commented Nov 2, 2024

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 list.idris = ... too and will stop highlightning if we rename to list.idris2 = ...?

@clason
Copy link

clason commented Nov 2, 2024

You are missing the point here. If the language name is idris (as specified in the grammar.js), you'll need to add it to nvim-treesitter under the same name. This has nothing to do with Vim filetypes; there is a separate function mapping between filetypes and languages.

@kayhide
Copy link
Owner

kayhide commented Nov 3, 2024

@srghma Thanks for the suggestions.

But first, please consider what @clason said.
If it does not solve your issue, come back here and let's think how to address it.

@srghma
Copy link
Collaborator Author

srghma commented Nov 3, 2024

ok here is my proposal

  1. rename this repo from https://github.com/kayhide/tree-sitter-idris to https://github.com/kayhide/tree-sitter-idris2
  2. merge srghma@901b55d to this repo
  3. merge nvim-treesitter/nvim-treesitter@803178d to feat(idris): add parser and queries nvim-treesitter/nvim-treesitter#7274
  4. to make everything work - I have added
      -- will make ```idris foo = 1 + 2 ``` in markdown work
      -- when idris3 will be released - just replace idris2 with idris3
      vim.treesitter.language.register('idris2', 'idris')

https://github.com/AstroNvim/astrocommunity/pull/1258/files

reason

maybe there will be idris 3 - https://discord.com/channels/827106007712661524/827106088343175180/1271141058289598619

proof that everything works

(in bottom highlight is different bc from lsp)

2024-11-03-09am-35-03-screenshot

@srghma
Copy link
Collaborator Author

srghma commented Nov 4, 2024

@clason why thumb down?

@clason
Copy link

clason commented Nov 4, 2024

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).

@srghma
Copy link
Collaborator Author

srghma commented Nov 4, 2024

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'?

@clason

@clason
Copy link

clason commented Nov 4, 2024

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.)

@srghma
Copy link
Collaborator Author

srghma commented Nov 4, 2024

@clason so, You propose

  1. treesitter - should work in files with idris filetype
  2. nvim-lsp
    a. (if ever will be released) idris-lsp in idris filetypes
    b. idris2-lsp should (and is) work in idris2 filetype
    c. (if ever will be released) idris3-lsp in idris2 filetypes
  3. marrying 1. and 2. together should be done in some pluin, e.g. https://github.com/AstroNvim/astrocommunity/pull/1258/files#diff-384113bdcc604154d09233d86ed93b355db3d4f98a6bb7b8f5635d9a64a1b840

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

@alexaandru
Copy link

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.

@clason
Copy link

clason commented Nov 5, 2024

🤓 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).

@clason
Copy link

clason commented Nov 5, 2024

But since the OP and the comments are a bit all over the place, let me summarize the situation from the Neovim side:

  1. No matter what the parser is called, things will work; there are standard methods in place to deal with parser name and filetype being different.
  2. That being said, the official filetype in Vim/Neovim is (now) idris2, so having the language be the same name would simplify things slighty. (But again, to be very clear, it's not a big deal either way. Lots of languages are in the same situation.)

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.

@alexaandru
Copy link

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 json5 ;-) You are right on jinja2 though.

@clason
Copy link

clason commented Nov 5, 2024

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 idris2, but that would have to be weighed against the (not small) cost of renaming an existing project.

@srghma
Copy link
Collaborator Author

srghma commented Nov 6, 2024

Umm, json5 is a really poor example

I dont like this too, I wish it was called just idris, but

  1. they have separate websites (bc this is a research language)
  2. everyone are naming their projects idris2-xxx now https://github.com/stefan-hoeck/idris2-filepath (what else can we do)

2024-11-06_07-35-05

@srghma
Copy link
Collaborator Author

srghma commented Nov 6, 2024

@kayhide Since vim/vim#15987 was merged and https://github.com/kayhide/tree-sitter-idris doesnt support idris2,
could You rename https://github.com/kayhide/tree-sitter-idris to https://github.com/kayhide/tree-sitter-idris (I dont have access)

and then I will make pr to rename name = "idris" to name = "idris2" in grammar.js

and we can close this issue

@kayhide
Copy link
Owner

kayhide commented Nov 8, 2024

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.
I just said that I have not tested for idris1. That does not exclude supporting idris1. If anybody willing to contribute supporting it, I am open to take it.

Aside the discussion above, it seems twisted to make such a change on parser's side for the sake of editor's convenience.
It reminds me a web browser updating for supporting non-standard htmls just because there exist such web pages in the wild. The result was disastrous.

@srghma
Copy link
Collaborator Author

srghma commented Nov 8, 2024

....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

au BufNewFile,BufRead *.idr			setf idris
au BufNewFile,BufRead *.lidr			setf lidris

?


I think yes, bc lean4 is just lean

https://github.com/vim/vim/blob/28d3941ede2995b76755ec1fab7f347bc1663b86/runtime/filetype.vim#L1326-L1327

@srghma srghma closed this as completed Nov 8, 2024
@clason
Copy link

clason commented Nov 8, 2024

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.

@kayhide
Copy link
Owner

kayhide commented Nov 8, 2024

@srghma Well, if you have a good reason for that change.
Your idea is not backward compatibie, meaning it causes loss to the users.
So at least, you need a reason which compensates the loss.
Otherwise, it is natural that your proposal getting not accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants