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

Upgrade to LSP 3.16 #112

Closed
wants to merge 8 commits into from
Closed

Upgrade to LSP 3.16 #112

wants to merge 8 commits into from

Conversation

illright
Copy link
Contributor

@illright illright commented Jan 2, 2021

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
Copy link
Member

UziTech commented Jan 8, 2021

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

@aminya
Copy link
Member

aminya commented Jan 27, 2021

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

@UziTech
Copy link
Member

UziTech commented Jan 27, 2021

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

@illright
Copy link
Contributor Author

Yeah, I'll polish it up soon

@aminya
Copy link
Member

aminya commented Feb 7, 2021

Any update on this?

@illright
Copy link
Contributor Author

illright commented Feb 7, 2021

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
Copy link
Member

aminya commented Feb 7, 2021

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
Copy link
Member

UziTech commented Feb 7, 2021

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
Copy link
Member

aminya commented Feb 7, 2021

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 aminya mentioned this pull request Feb 7, 2021
@aminya
Copy link
Member

aminya commented Feb 7, 2021

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
Copy link
Member

UziTech commented Feb 7, 2021

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

Copy link
Member

@aminya aminya left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions.

I am closing this PR as I have been able to extract the following PRs out of this. Various modifications were applied, and I have added the tests for the new features.

As a suggestion for your future contributions, if you can divide your contributions into separate small incremental changes, it would make it much easier for us to review, test, and verify. As an example, this PR had a hidden change that could break the whole Autocomplete for the clients.

@aminya aminya closed this Mar 19, 2021
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

Successfully merging this pull request may close these issues.

Update jsonrpc and languageserver dependencies
3 participants