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

[RFC] Extract LSP protocol-only code to a separate shard #54

Closed
asterite opened this issue Nov 30, 2022 · 5 comments
Closed

[RFC] Extract LSP protocol-only code to a separate shard #54

asterite opened this issue Nov 30, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@asterite
Copy link
Collaborator

Extracted from #53

I think it would be nice to extract the code that's exclusive to LSP to a separate shard. The advantages of doing that are:

  1. Clearer separation of concerns
  2. Less code in this repository, making it more accessible
  3. Avoiding duplicating code and effort (there's already another repo for this)
  4. Allowing others to use the LSP shard for other language server implementations (not only Crystal ones. For example there could be one for Mint).

One possible place for this new repo could be in elbywan. Another place could be crystal-community to encourage contributions.

@elbywan
Copy link
Owner

elbywan commented Dec 1, 2022

I think it would be nice to extract the code that's exclusive to LSP to a separate shard.

Yes definitely, for the time being under elbywan because it is easier for me. In the long run, I have no issues with it being ported over to the crystal-community org (it is gonna be MIT licensed).

It should be pretty quick and I can work on it in the following days + a PR to make crystalline use the shard.

On a side note, extracting the LSP should not be blocking any other topics since it does not change anything internally to import it or use the embedded version

@elbywan elbywan self-assigned this Dec 1, 2022
@elbywan elbywan added the enhancement New feature or request label Dec 1, 2022
@asterite
Copy link
Collaborator Author

asterite commented Dec 1, 2022

On a side note, extracting the LSP should not be blocking any other topics since it does not change anything internally to import it or use the embedded version

Definitely!

Another thing I was thinking is that right now types aren't defined in places where it's idiomatic to find them. For example Crystalline::Diagnostics is at src/crystalline/classes/diagnostics.cr instead of src/crystalline/diagnostics.cr and it takes a few seconds/minutes to realize this. So moving LSP to a shard would also allow moving things directly into src/crystalline (well, I guess that could be done at any time.)

@elbywan
Copy link
Owner

elbywan commented Dec 3, 2022

There we go for the extracted shard: https://github.com/elbywan/crystal-lsp

Another thing I was thinking is that right now types aren't defined in places where it's idiomatic to find them. For example Crystalline::Diagnostics is at src/crystalline/classes/diagnostics.cr instead of src/crystalline/diagnostics.cr and it takes a few seconds/minutes to realize this. So moving LSP to a shard would also allow moving things directly into src/crystalline (well, I guess that could be done at any time.)

Sure! I will handle this in the follow up pull request to plug the new shard into crystalline.

@asterite
Copy link
Collaborator Author

asterite commented Dec 4, 2022

Thank you, this is fantastic! I really like it that you also extracted the server and the loop, so an implementor just needs to define the on_... methods. In my proposal I just had in mind the types that get transmitted over the protocol, not the protocol implementation, but in retrospective what you did is much better 💙

@elbywan
Copy link
Owner

elbywan commented Dec 5, 2022

Thank you, this is fantastic! I really like it that you also extracted the server and the loop, so an implementor just needs to define the on_... methods. In my proposal I just had in mind the types that get transmitted over the protocol, not the protocol implementation, but in retrospective what you did is much better 💙

Thanks! This is the way I designed the LSP part from the start, so people can reuse the server if they don't want to reinvent the wheel 😉.

And there we go for the follow-up PR integrating the shard and cleaning up things a bit: #56

So I think we should be good regarding the LSP side now!

@elbywan elbywan closed this as completed Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants