search

atom-community / atom-languageclient

Provide integration support for adding Language Server Protocol servers to Atom.
https://www.npmjs.com/package/atom-languageclient
MIT License
45 stars 13 forks source link

Upgrade to LSP 3.16 #112

Closed illright closed 3 years ago

illright commented 3 years ago

I'm currently working on upgrading the LSP version. For now I have just set up some foundations, fixed compiler errors from the update, and now I'm gonna start implementing the new features. Figured it'd be good to get feedback as I'm working since this is my first dive into the Atom ecosystem

Closes #110

UziTech commented 3 years ago

I merged @types/atom v1.40.6 if you rebase that should help with typescript.

aminya commented 3 years ago

Can we get the tests working and merge this for now? We can add other features in separate PRs.

UziTech commented 3 years ago

I think we should also get tests written for this before merging.

illright commented 3 years ago

Yeah, I'll polish it up soon

aminya commented 3 years ago

Any update on this?

illright commented 3 years ago

Haven't had much time these past days. I'm a bit lost with the tests, honestly, because I could simply fix the failing tests and be done with it, but we wanna do it properly and create tests for new functionality. That's where I fall into the endless loophole of trying to create a thorough test suite, realizing that the surrounding tests aren't thorough enough and/or need to be updated.

Besides, a lot of stuff isn't finished here, and I don't even know if this PR is of any value as of now, apart from all the scaffolding that is meant to become functionality in the future. Is there a reason why you want to merge this soon?

aminya commented 3 years ago

If you can make the build working and make the current tests pass, we can merge this to a dev branch, and add the new features and extra tests for the new features in other pull requests.

When a PR becomes big, it is hard to review and test, and it also blocks other improvements. We want to continue developing this package, and because this PR is editing multiple files, it is hard to do this without introducing conflicts.

UziTech commented 3 years ago

tests for the new features in other pull requests

No we should not merge this without proper tests. The tests make it easier to review this since we can know that it is working the way it is supposed to.

aminya commented 3 years ago

It should only add tests for the new features.

I suggest separating the concerns. This PR should only focus on upgrading LSP (the first commit), not adding new features. Each new feature should be in a separate PR, so it can be tested properly.

aminya commented 3 years ago

I added a PR (#122) which bumps the LSP.

Please cherry-pick the separate features, and open separate PRs for each (including the tests). This byte size approach makes everything easier and reduces the risk of bugs.

UziTech commented 3 years ago

I agree, smaller PRs are easier to test and review, but we still need tests for any new features and updates.