sublimelsp / lsp_utils

Module with LSP-related utilities for Sublime Text
https://sublimelsp.github.io/lsp_utils/
MIT License
16 stars 6 forks source link

Create NpmClientHandler to simplify NPM handler creation #5

Closed rchl closed 4 years ago

rchl commented 4 years ago

Adds NpmClientHandler export that at minimium requires only overriding PACKAGE_NAME, SERVER_DIRECTORY and SERVER_BINARY_PATH class attributes.

rchl commented 4 years ago

As an example, LSP-eslint would look like this when using new class:

import os
import webbrowser

from lsp_utils import NpmClientHandler

def plugin_loaded():
    LspEslintPlugin.setup()

def plugin_unloaded():
    LspEslintPlugin.cleanup()

class LspEslintPlugin(NpmClientHandler):
    package_name = __package__
    server_directory = 'vscode-eslint'
    server_binary_path = os.path.join(server_directory, 'out', 'eslintServer.js')

    def on_ready(self, api) -> None:
        api.on_notification('eslint/status', self.handle_status)
        api.on_request('eslint/openDoc', self.handle_open_doc)

    def handle_status(self, params) -> None:
        pass

    def handle_open_doc(self, params, respond) -> None:
        webbrowser.open(params['url'])
        respond({})

@jfcherng @deathaxe @RandomByte

rchl commented 4 years ago

I've checked all npm-based LSP-* packages and this design should address all of them.

We could abstract it even more - hide the client and provide abstractions for client.on_notification/on_request/send_response methods. Then, when the LSP API changes, we could address all of that within lsp_utils without changing packages (in theory).

rchl commented 4 years ago

There is a problem. LSP initializes subclasses of LanguageHandler but we don't want to initialize NpmClientHandler (which is a direct subclass of LanguageHandler) but only subclasses of NpmClientHandler. So this either needs a solution in LSP itself or something else that I can't think of.

deathaxe commented 4 years ago

Just out of couriosity: Why do we need a separate dependency to provide a class which is derived from an abstract interface class the LSP package?

I understand its purpose and what it does, but why can't such infrastructure not be part of the LSP package?

The dependency needs to import from LSP, any LSP-blabla package needs to import the dependency and other classes from LSP. All in all everything requires LSP package to be available first for all that stuff to work.

It feels pretty complicated.

rchl commented 4 years ago

I understand its purpose and what it does, but why can't such infrastructure not be part of the LSP package?

I think that's because as much as LSP doesn't want to be concerned with package-specific behaviors and settings, it also wouldn't want to be concerned with various ways of handling their installation and dependency management.

rchl commented 4 years ago

There is a problem. LSP initializes subclasses of LanguageHandler but we don't want to initialize NpmClientHandler (which is a direct subclass of LanguageHandler) but only subclasses of NpmClientHandler. So this either needs a solution in LSP itself or something else that I can't think of.

Addressing in https://github.com/sublimelsp/LSP/pull/1000