Add a `LanguageClient` autogenerated from `lsprotocol` type definitions

[alcarney]

Description (e.g. "Related to ...", etc.)

This is something I'd originally put together for pytest-lsp but thought it was probably useful enough to try upstreaming into pygls :slightly_smiling_face:

• This adds a gen_client.py script that will autogenerate a LanguageClient in pygls/lsp/client.py based on the type definitions in lsprotocol. It can easily be run via tox - $ tox -e generate
• The client is very simple and contains no logic. It only contains sync and async method definitions for all requests/notifications that originate from the client.
• LanguageClient is based on a new base Client class that currently only implements a start_io method.
• This PR also updates the test suite to use the new LanguageClient

I'm also hoping this PR can help move the discussion forward in #306, perhaps even agreeing on pattern we can use for a new, autogenerated base server class? Obviously there's more logic to a server which we won't be able to generate, but we should at least be able to automate the boilerplate method definitions etc.

I'm also using the new base client in pygls/client.py as an opportunity to try the higher level asyncio APIs which should allow us to simplify some of the code in pygls/server.py

Let me know your thoughts :smile:

Code review checklist (for code reviewer to complete)

• [x] Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• [x] Title summarizes what is changing
• [x] Commit messages are meaningful (see this for details)
• [x] Tests have been included and/or updated, as appropriate
• [x] Docstrings have been included and/or updated, as appropriate
• [ ] Standalone docs have been updated accordingly
• [/] CONTRIBUTORS.md was updated, as appropriate
• [/] Changelog has been updated, as needed (see CHANGELOG.md)

[tombh]

Always the visionary!

I don't think this is as big a change as the Files Changed count makes out? Currently it just affects the client, which is currently only used for testing? But it is of course also laying the groundwork for a similar approach to the server, both in terms of autogeneration and introducing asyncio.

So a few questions that come to mind:

• What was the initial impetus that made you want to do this in pytest-lsp? Of course I can appreciate the LSP Joy of autocompleted method names and signatures 🤓 But I imagine there's more to it than that?
• Does this add weight to the need for version pinning lsprotocol? The autogenerated client will be generated from a specific lsprotocol version so Pygls should also formally depend on that version?
• Unrelated ... I'm curious about the formatting changes in this PR. I think we get some CI-enforced linting through flake8? But it's lenient enough to allow various styles of formatting, eg ' versus ", etc? I don't have any opinion on what style we use, just that we stick to one style. I've always just used Black, maybe that's an option?

[alcarney]

I don't think this is as big a change as the Files Changed count makes out?

The bulk of the files changed are in https://github.com/openlawlibrary/pygls/pull/328/commits/e240cc765667573bf5db177debdc98dc9aba4d7a which is just tweaking the test suite to make use of the new client.

What was the initial impetus that made you want to do this in pytest-lsp?

Pre v1, pygls didn't parse server responses into objects so I was originally doing the conversion in pytest-lsp manually. Thanks to the lsprotocol integration, that's no longer necessary so it's now mostly about discoverability and reducing some of the boilerplate in sending requests.

Does this add weight to the need for version pinning lsprotocol?

Now that lsprotocol runs pygls' tests suite as part of it's CI, I wonder if version pinning will be less of a concern? Since the client only depends on lsprotocol for type annotations and the spec itself can't change too drastically I image the risk of incompatibilities between versions will be low anyway.

I'm curious about the formatting changes in this PR.

Sorry about that :grimacing:, I have my editor setup to auto format files and since this was just a draft, I hadn't gone around and cleaned up the formatting yet.

I'd be happy to go with black - maybe in combination with isort? I'm not fussy about style either, I only care about it being automated! :smile: Any thoughts on setting up pre-commit on this repo?

[tombh]

Great, thank you. I feel I've got a better grasp on this now. It clearly feels like a logical step for the post-v1, lsprotocol-enabled Pygls. We get better testing and better code discoverability.

Regarding lsprotocol pinning, it's only testing against the latest Pygls right? So we do indeed have the guarantee that latest Pygls always works against latest lsprotocol. But of course Pygls-based LSs might be pinning to a particular pygls version, where Pygls itself is pulling in the latest lsprotocol. Unless of course the LS is using some kind of lock file. So yes I think the odds of incompatibilities are lower now, but I wonder if the downsides of breakage because of version incompatibilities are worse than the downsides of pinning against lsprotocol (namely; having to release Pygls more often and losing out on automatically included improvements)?

Don't stress too much about the formatting right now. I'll look into Black and isort at some point.

[alcarney]

I've managed to revert most of the formatting only changes - it should be a lot more obvious what's actually changed now :)

but I wonder if the downsides of breakage because of version incompatibilities are worse than the downsides of pinning against lsprotocol

Personally, I think it would be a mistake to introduce anything stronger than a lower bound on the version number of lsprotocol. Here's my reasoning

• pygls is a library so it should try to impose as few package constraints as possible
• Locking pygls to a single/maximum lsprotocol version forces servers to upgrade pygls if they want the latest lsprotocol - potentially forcing them to accept changes from us that they're not ready for. Particularly frustrating if they know the new version of lsprotocol would work fine with their existing version of pygls in reality.
• Even though lsprotocol would only be testing against the latest pygls, I think that's enough to ensure compatibility due to the spec itself having to be backwards compatible - most significant changes are going to be additive

Unless of course the LS is using some kind of lock file

This. Ultimately, if a server needs a specific lsprotocol, pygls combo, it's on the server to specify those requirements.

[tombh]

Nice work on reverting the formatting changes! I wouldn't have held it against you if they'd remained. But still thanks 🙇

I actually thought a whole lot about your arguments against version pinning, and ended up writing a mini essay 🤓 https://github.com/openlawlibrary/pygls/issues/331 Long story short: I'm happy not to pin.

[alcarney]

LanguageClient is based on a new base Client class that currently only implements a start_io method.

I rewrote this method to be based on asyncio.create_subprocess_exec instead of it basically being a copy of start_io from the server. Hopefully this makes the class much easier to use as the Client itself is now responsible for managing the server process and not the user.

Also by using an asyncio subprocess, we get async readers/writers for stdout/stdin - removing the need for the thread pool!

This PR also updates the test suite to use the new LanguageClient

I've reverted that and instead opted for a different approach that I hope makes sense :)

This commit introduces a new example language server! examples/servers/code_actions.py It's a toy server that uses code actions to write in the solutions to some simple sums (see examples/workspace/sums.txt).

This server is then tested with what I think is pygls' first true end-to-end test, driven by the new LanguageClient, replacing the old code actions test in tests/lsp/test_code_actions.py

What I'm thinking is that we can slowly start introducing end-to-end tests covering parts of the LSP spec, while simultaneously building up a set of concrete examples of how each part of the spec works.

The one downside is that these end-to-end tests have to be skipped in pyodide since the architecture of the test runner won't work for that use-case, but hopefully we can fix that at some point^TM

[tombh]

So we have server:

left = int(match.group(1))
right = int(match.group(2))
answer = left + right

And a real client that tests that server!

assert fix.new_text == "1 + 1 = 2!"

This is so good. Thank you so much for this. I think it's great for testing and implicit "documentation". Every test basically becomes a readable example now 🤓

So next time I have to write or touch a test, I'll upgrade it to the new way.

And great work with the async stuff, it's not a huge amount of line changes, but a huge amount of investigating went into it.