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 flexibility for extending the LanguageClient #195

Open
eliericha opened this issue Nov 25, 2024 · 4 comments
Open

Add flexibility for extending the LanguageClient #195

eliericha opened this issue Nov 25, 2024 · 4 comments
Labels
enhancement New feature or request pytest-lsp Issues affecting the pytest-lsp package

Comments

@eliericha
Copy link

The API is made so that many aspects are customizable, and that's great! However I hit the following limitation.

ClientServerConfig can be given a client_factory to replace make_test_lsp_client. That allows me to inject my own class that extends LanguageClient.

However make_test_lsp_client does the registration of the handlers for capturing diagnostics, progress reporting, etc... I still want to benefit from that and currently I cannot reuse that.

So would it be possible to move the registration of handlers to a reusable method that takes the client as a parameter?

Alternatively, could make_test_lsp_client be given a factory function to instantiate the LanguageClient object? That would allow me to pass my own class.

Thanks!

@eliericha
Copy link
Author

eliericha commented Nov 26, 2024

Actually I tried copying make_test_lsp_client to use my own LanguageClient and hit an issue:

def my_client_factory() -> MyLanguageClient:
    client = MyLanguageClient(
        converter_factory=default_converter,
    )

    @client.feature(types.WORKSPACE_CONFIGURATION)
    def configuration(client: LanguageClient, params: types.ConfigurationParams):
        return [
            client.get_configuration(section=item.section, scope_uri=item.scope_uri)
            for item in params.items
        ]

    ...

This works initially, but fails later when the feature handler is called. The first argument of the handler has type LanguageClient while the client type is MyLanguageClient. The generic handler infrastructure is unable to determine that it should pass the client as the first parameter because the types don't match exactly, and thus it makes a call to the handler with one parameter missing --> exception.

This means that it will be difficult/impossible to extract the feature registration code to a function that accepts any class extending LanguageClient.

@alcarney
Copy link
Member

alcarney commented Nov 26, 2024

This works initially, but fails later when the feature handler is
called. The first argument of the handler has type LanguageClient
while the client type is MyLanguageClient. The generic handler
infrastructure is unable to determine that it should pass the client
as the first parameter because the types don't match exactly, and thus
it makes a call to the handler with one parameter missing -->
exception.

That definitely sounds like a bug report for pygls (which provides the feature registration system).

However, I'm fairly sure you can work around the issue for now by renaming your client: LanguageClient parameter to ls: LanguageClient. (See upstream docs)

@alcarney
Copy link
Member

But yes, I agree there should be a mechanism to make it easy to use a custom client class.

@alcarney alcarney added enhancement New feature or request pytest-lsp Issues affecting the pytest-lsp package labels Nov 26, 2024
@eliericha
Copy link
Author

I see. Thanks for the doc pointer!

Indeed renaming the parameter to ls makes it work. And a quick debug session allowed me to understand the source of the bug :) I'll report the findings on the pygls issue.

So now there is nothing blocking an API change in pytest-lsp to support extending LanguageClient.

I suggest simply moving the feature registration code into a register_test_client_lsp_features(ls : LanguageClient).

That would allow me to provide a client_factory that instantiates my class, and defers to register_test_client_lsp_features for reuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pytest-lsp Issues affecting the pytest-lsp package
Projects
None yet
Development

No branches or pull requests

2 participants