Skip to content

LSP Backend #1739

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

Merged
merged 24 commits into from
Jun 25, 2024
Merged

LSP Backend #1739

merged 24 commits into from
Jun 25, 2024

Conversation

FastestMolasses
Copy link
Member

Description

Contains the LSP backend.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

@FastestMolasses FastestMolasses added this to the v0.2.0 milestone May 31, 2024
@FastestMolasses FastestMolasses added enhancement New feature or request architecture labels May 31, 2024
@FastestMolasses FastestMolasses self-assigned this May 31, 2024
Copy link
Contributor

@matthijseikelenboom matthijseikelenboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good man. I've left some notes on things that have to change and some that should be discussed

@FastestMolasses FastestMolasses marked this pull request as ready for review June 22, 2024 20:50
bombardier200
bombardier200 previously approved these changes Jun 22, 2024
Copy link
Member

@tom-ludwig tom-ludwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I'm really excited about getting all this functionality fully implemented. I found just a few small issues. We're still missing tests, and I'm not sure how to mock good responses from the language server or set one up for testing from scratch. Also, could you rename the folder LanguageServer+ to something like LanguageServerExtensions?

@FastestMolasses
Copy link
Member Author

I'll take a look at adding a mock provider for tests

@FastestMolasses
Copy link
Member Author

So I think one way we could do it is by creating a fake server binary. It would be a swift script that gets compiled and it would send back a randomized json response based on the request it received. Can I get some thoughts on this before I proceed with it? It would be a big task to get a mock language server that can effectively test the implementation. @CodeEditApp/maintainers

@tom-ludwig
Copy link
Member

No, that would be a waste of time right now. Tests are crucial in CE to ensure everything works now and after others make changes, especially with tightly coupled code that many people use and modify. But we’re not even using your language server functions yet. If setting up tests for server binaries is too complicated or time-consuming, I'd say skip them for now. We definitely need to create them eventually😅

@FastestMolasses
Copy link
Member Author

Ok I'll make an issue for it and I can do it in the next milestone.

@tom-ludwig tom-ludwig merged commit c78ef96 into main Jun 25, 2024
2 checks passed
@tom-ludwig tom-ludwig deleted the lsp-provider branch June 25, 2024 17:20
Copy link
Contributor

@matthijseikelenboom matthijseikelenboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it's already merged, but still one point. (Didn't have to be done in this PR tho)


struct NoExtraData: Hashable { }

struct CacheKey: Hashable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we need to do something with this. CacheKey is now a callable struct from within the entire project, but it doesn't describe what it does at all.

Either should be fixed by moving to modules or very explicit documentation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the naming more clear in my next or

@thecoolwinter thecoolwinter added the language server Issues or Pull Requests related to language servers. label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture enhancement New feature or request language server Issues or Pull Requests related to language servers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants