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

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

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

oliversen
Copy link
Contributor

@oliversen oliversen commented Jul 18, 2023

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)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • 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
Copy link
Collaborator

tombh commented Jul 18, 2023

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

This seems to make sense, there's similar pattern with

def create(self, token: ProgressToken, callback=None) -> Future:

and
async def create_async(self, token: ProgressToken) -> asyncio.Future:

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

@oliversen
Copy link
Contributor Author

oliversen commented Jul 22, 2023

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.

def get_configuration(self, params: WorkspaceConfigurationParams,

def get_configuration_async(self, params: WorkspaceConfigurationParams) -> asyncio.Future:

def show_document(self, params: ShowDocumentParams,

def show_document_async(self, params: ShowDocumentParams) -> asyncio.Future:

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

tombh commented Jul 23, 2023

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

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

bump @oliversen

can I help?

@oliversen oliversen force-pushed the add-await-syntax-support branch from aa9251a to 90b6bba Compare September 1, 2023 16:12
@oliversen
Copy link
Contributor Author

bump @oliversen

Done!

@tombh
Copy link
Collaborator

tombh commented Sep 1, 2023

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.

@oliversen oliversen force-pushed the add-await-syntax-support branch from 90b6bba to 939f1a4 Compare September 2, 2023 17:03
@tombh
Copy link
Collaborator

tombh commented Sep 3, 2023

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

@oliversen
Copy link
Contributor Author

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

Very strange, I have no problems with formatting.
black

@tombh
Copy link
Collaborator

tombh commented Sep 3, 2023

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?

@oliversen oliversen force-pushed the add-await-syntax-support branch from 939f1a4 to 637d3fd Compare September 3, 2023 18:37
@tombh tombh self-requested a review September 3, 2023 19:45
@tombh tombh merged commit 3cb554a into openlawlibrary:main Sep 3, 2023
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.

4 participants