Open alcarney opened 1 year ago
I might just try migrating pygls over anyway to see if it would even work, but it's also probably worth checking to see what people's thoughts were on this :slightly_smiling_face:
That's a lot of debugging! Really awesome that you're getting to the bottom of this. I think it'd be great to modernise Pygls' async code. Though of course I'm not deeply familiar with the current design decisions. The fact that you've built up so much momentum in this area now, I'm sure you'd do a great job of migrating to the high level APIs.
Ok, as promised in #375, some thoughts on where I'd like to try and take this.
JsonRPCProtocol
JsonRPCProtocol
to derive from asyncio.Protocol
anymoreReader
and Writer
objects, we should be able to reuse the same main loop for everything and stop parsing messages twiceBaseTransport
JsonRPCProtocol
be based on asyncio.Tasks
instead of futures, but I'm fully prepared to back off from this if looks like we'd lose something significant Servers
BaseLanguageServer
(just like BaseLanguageClient
), that way we should have consistent methods etc and resolve #306, by removing the need for methods to be implemented on LanguageServerProtocol
itself.JsonRPCServer
class (mirroring JsonRPCClient
) that replaces Server
and uses the high-level asyncio APIsI'm sure I've forgotten a few things but those should be the headlines.
I appreciate it will take some time to implement all of that! I was thinking we could develop most, if not all of this side-by-side with the existing stuff. That way we can incrementally ship the new approach and get feedback, then, (assuming it all works out!) deprecate and eventually remove the existing approach in a v2 release.
I'm not saying I want to rewrite the world! But in cleaning up some of the inconsistencies we will wind up introducing some breaking changes - but we can try to make them as minimal as possible.
Let me know your thoughts! :smile:
I don't have any particular technical feedback here. The main thing is that I totally support a v2. All your suggestions sound really positive and that they have wider benefits to the codebase. Pygls doesn't actually have a particularly large codebase, so maybe rewriting the world isn't such a bad perspective to have!
My initial reason for separating protocol.py
into smaller, class-based files (apart from just making it more digestible) was to make it easier to piecemeal add strict typing. I was wondering how much of your suggestions here I could contribute to as I was doing that, but I'm not sure I have enough of an insight. But I'll definitely be open to it. And even if a lot of the code is just going to be replaced anyway, I'll add the types anyway, I don't want to put any expectations on you.
Have an auto-generated BaseLanguageServer (just like BaseLanguageClient)
This sounds awesome 🤓
BTW, did we ever talk about putting the client code in the test
folder? Not saying we should, just wondering about your thoughts on it. My thinking is that it's only use is for testing, so it would make more sense amongst the other test code. But it probably has other uses right?
I was wondering how much of your suggestions here I could contribute to as I was doing that, but I'm not sure I have enough of an insight. But I'll definitely be open to it.
Great thanks! Let me know if you want me to expand on anything
BTW, did we ever talk about putting the client code in the
test
folder?
Possibly? :thinking:
I think it's fine where it is, it's a solid base for the client side of the LSP protocol and while pytest-lsp
might be the primary consumer of it for now. There's nothing stopping someone from attempting to create a "proper" language client based off of it :smile:
I've been struggling to track down the source of this DeprecationWarning when using
pytest-lsp
with the latestpytest-asyncio
Long story short, I think I've finally tracked it down to the following block of code.
https://github.com/openlawlibrary/pygls/blob/fad812aae28bb4884b4877a5b986970941b57037/pygls/server.py#L187-L192
When giving
pygls
a specificloop
to use, it goes ahead and creates a new one anyway, resulting in a second, unused event loop thatpytest-asyncio
is correctly warning about. While tweaking the code above to something likeappears to resolve the issue. I do wonder whether it's worth revisiting how
pygls
handles event loops more generally.As far as I can tell, most of pygls' event loop code was written pre-Python 3.7 where the current high level asyncio APIs didn't exist.
I guess is this is a long winded way of asking - can anyone think of any use cases that require
pygls
to stick with the current low-level APIs? I admit, despitepytest-lsp
forcing me to care more about event loops, I still don't fully understand them :sweat_smile: