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

Explore parity between server.LanguageServer and protocol.LanguageServerProtocol methods #306

Closed
tombh opened this issue Dec 21, 2022 · 2 comments · Fixed by #489
Closed

Comments

@tombh
Copy link
Collaborator

tombh commented Dec 21, 2022

In server.LanguageServer we have a lot of methods that look like:

def apply_edit(
        self, edit: WorkspaceEdit, label: Optional[str] = None
    ) -> WorkspaceApplyEditResponse:
        """Sends apply edit request to the client."""
        return self.lsp.apply_edit(edit, label)

Therefore, they do nothing but call out to the underlying protocol.LanguageServerProtocol method. There may be a good reason for this, but it isn't documented. There are some cases such as with publish_diagnostics, where there is an advantage because the server.LanguageServer method signature is simpler.

Let's either document the reasoning for the apparent repetition, or deprecate the duplicated server versions in favour of the protocol versions.

@alcarney
Copy link
Collaborator

or deprecate the duplicated server versions in favour of the protocol versions.

Do you have any thoughts on the opposite?

Would it make sense to try and simplify the protocol classes (possibly even try to remove LanguageServerProtocol completely??) and instead keep the methods on the LanguageServer class and have them call the base notify, send_request, send_response methods on the JsonRPCProtocol class?

@tombh
Copy link
Collaborator Author

tombh commented Mar 19, 2023

Sorry for the late reply. The short answer is I don't know enough to have an opinion. I would very much trust your intuition on this. Will be interesting to see what #328 and its successors bring to the table.

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 a pull request may close this issue.

2 participants