openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
568 stars 103 forks source link

Add `await` syntax support for sending edit request to client #350

Closed oliversen closed 1 year ago

oliversen commented 1 year ago

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

Before:

def edit_callback(future):
    result = future.result()
ls.apply_edit(workspace_edit).add_done_callback(edit_callback)

After:

result = await ls.apply_edit_async(workspace_edit)

Code review checklist (for code reviewer to complete)

tombh commented 1 year ago

Thanks for the PR. Is this just a quality of life thing? As in await/async is just more conventional? Or is there a deeper more practical reason?

I wonder if this could be applied as default to all calls that use send_request?

perrinjerome commented 1 year ago

This seems to make sense, there's similar pattern with https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/progress.py#L36 and https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/progress.py#L52

I'm wondering, why not define apply_edit_async with async def ?

oliversen commented 1 year ago

Is this just a quality of life thing? As in await/async is just more conventional? Or is there a deeper more practical reason?

The code is cleaner, simpler, clearer (see issue description).

On the practical side we can form a response for the client, depending on the result of editing the workspace.

from pygls import server

LSP_SERVER = server.LanguageServer(...)

@LSP_SERVER.command('some_command')
async def some_command(ls: server.LanguageServer):
    ...
    result = await ls.apply_edit_async(WorkspaceEdit(...))
    if result.applied:
        response = ResponseMessage(...)
    else:
        response = ResponseMessage(...)
    return response

I'm wondering, why not define apply_edit_async with async def ?

No, async def is not needed in this case.

https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/server.py#L390

https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/server.py#L395

https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/server.py#L452

https://github.com/openlawlibrary/pygls/blob/464846b1205db087b66624b0e08357ef2703dfd7/pygls/server.py#L457

I wonder if this could be applied as default to all calls that use send_request?

How to understand "applied by default"? Features and commands can be registered as synchronous and asynchronous functions. All methods that use send_request() already have an asynchronous counterpart, except apply_edit().

tombh commented 1 year ago

Ohh, so you're just filling in a missing part of a convention we already have. That makes total sense. Ok, LGTM 🚢

Regarding the async def syntax, that would certainly make more sense to me, but then I'm not that familiar with the asyncio.Future pattern. And besides, this PR is simply about completing a pattern that we are already using elsewhere. Maybe we can convert to the async def pattern another time.

It looks like you just need to rebase main and I we can get this merged. Thank you ✨

RossBencina commented 1 year ago

It looks like you just need to rebase main and I we can get this merged. Thank you ✨

bump @oliversen

can I help?

oliversen commented 1 year ago

bump @oliversen

Done!

tombh commented 1 year ago

The CI lint error wants a commit message more like feat: add await syntax support for sending edit request to client.

And a rebase from main to get the latest changes.

tombh commented 1 year ago

Sorry I didn't notice before, but the commit also needs formatting with black.

oliversen commented 1 year ago

Sorry I didn't notice before, but the commit also needs formatting with black.

Very strange, I have no problems with formatting. black

tombh commented 1 year ago

I love your prompt 🥹

This is the diff I'm seeing black ask for:

diff --git a/pygls/protocol.py b/pygls/protocol.py
index 62b1f8a..28b7d90 100644
--- a/pygls/protocol.py
+++ b/pygls/protocol.py
@@ -746,8 +746,9 @@ class LanguageServerProtocol(JsonRPCProtocol, metaclass=LSPMeta):
         self, edit: WorkspaceEdit, label: Optional[str] = None
     ) -> WorkspaceApplyEditResponse:
         """Sends apply edit request to the client. Should be called with `await`"""
-        return self.send_request_async(WORKSPACE_APPLY_EDIT,
-                                       ApplyWorkspaceEditParams(edit=edit, label=label))
+        return self.send_request_async(
+            WORKSPACE_APPLY_EDIT, ApplyWorkspaceEditParams(edit=edit, label=label)
+        )

     @lsp_method(EXIT)
     def lsp_exit(self, *args) -> None:

I see your prompt is saying it's on branch main, maybe you need to checkout the add-await-syntax-support branch?