Introduce LspPlugin API

[rchl]

"Introduce new plugin API" is a little over the top. I just wanted to jump-start it with one new method to allow for progressive enhancement and get early feedback.

The new handle_update_or_installation_async method combines needs_update_or_installation and install_or_update. Those were split because the code sets installing... status text if needs_update_or_installation returns true but that meant that various if checks had to be duplicated across those two and generally made the logic harder to read. With this one method, there is now a callable function passed that can be used to start the progress (perhaps unnecessary since user could use configuration.set_view_status but then that would likely lead to inconsistent status messages across packages).

The new method takes a dict with params. This is to allow adding new params without breaking API compatibility.

Most class methods are passed PluginContext now with configuration instance now to allow packages to get resolved configuration. Some packages currently read settings with sublime.load_settings but that has the problem of not considering project overrides.

Resolves #2039
Resolves #2491

[rchl]

Actually just dumped some old code that I had in stash.

[rchl]

I've been thinking about case that tends to come up often - people want to use non-managed server binary.

Overriding command is one way to do it and it would work for simple cases but in more complex cases like LSP-clangd where there is a lot of arguments and some are even programmatically adjusted during start, it wouldn't work.

So my thought is that, given we use a variable like $server_binary, user should be able to override just the variables from the config file. It would need to override variables added through the additional_variables API.

Just a little concerned that it's not the simplest to explain to the user. Or at least will require a bunch of extra doc in every package but that's the best I can think of.

[jwortmann]

@predragnikolic Do you maybe see anything else for the LspPlugin class in api.py that could be improved? Like the method names, or the parameter names and types, or is anything else missing?

I think it looks mostly good from my side now, but we should try to get it "perfect" now, so that we don't need any breaking changes or awkward additions later.

[predragnikolic]

I will have some free time at the begging of next week. (Mon, Tue)

I'll try to find time to go through the changes and try to migrate some plugins to try out the new API (I will do it only locally).

Maybe I will return with some feedback.
Else if I cross that timeline I would suggest to not wait on me.
And maybe transition one the most complicated plugins first just to see if something seems like it needs additional tweaking.

[rchl]

I'm toying with the idea of making ClientConfig fully typed but discussing it here could get messy so created discussion for it - #2860

Feel free to chime in with opinions or better ideas.

[rchl]

I think I should also add a method for extending env to support this functionality from lsp_utils.

[rchl]

I should probably add something like below to be consistent but man I don't like it...

    @classmethod
    def env(cls, context: PluginContext) -> dict[str, str]:
        return context.configuration.env

Now in the override one has multiple options:

• call super() to get the value, then update it and then return
• update and return context.configuration.env
• update context.configuration.env but return empty object
• return new object with custom env.

I don't like it because it's so unclear how LSP handles the returned value. Does it merge or override existing values?
Of course all can be explained in the documentation of the method but it should be just clearer without reading docs.

If there would be a method that just has context as an argument, returns None and lets user do anything to the ClientConfig then at least it wouldn't leave any room for misunderstanding.

[rchl]

Technically one can just modify env directly from install_async so I think we are fine without extra methods.

In that case the same could be said about command and initialization_options methods. It just makes it more messy for implementations to separate the logic into those when it can be done from basically any static method.

Opinions?

[predragnikolic]

Currently some LSP-* plugins rely on lsp_utils and LSP,
It would be nice if the new API completely replaced lsp_utils, and made the plugins only rely on LSP.

Resasons:

• lsp_utils is sometimes requires to be in sync with the latest changes in LSP, and PC sometimes can causes issues because it doesn't install dependecies when we expect it.
• Doing local development of dependencies is hard to setup in ST for some reason. (I have some steps in some discord thread on what I have to do in order to do local development for lsp_utils and not let it be overriden by PC, and also I have not really followed the thread on how to publish lsp_utils now...) To much steps. But I see having that code in LSP would ease maintenance.

This might meant that we move code from lsp_utils in LSP, but not sure how other LSP maintainers would feel about it. (but as said it would ease maintenance of lsp_utils, with the cost of moving it into LSP and shipping unusued code if people did not use some of those plugins)

[predragnikolic]

If we add file pattern support in the future
It would be nice to think in advance and see if the current API design would make it easy to add file patterns sometimes in the future.

For example, instead of currently only supporting selector, think if the current API is nice, and will allow adding on_uri: NotRequired[list[str]] (If specified the server will only start for the given uri. and will shutdown as soon as the last view that matches the uri is closed.) (https://github.com/zed-industries/package-version-server should only be started in package.json files)

and workspace_contains: NotRequired[list[str]] in a nice way. (lsp-ast-grep should only start if the server if the workspace contains a configuration file, tailwind css should only start in a workspace that has tailwindcss configured)

[predragnikolic]

for me on_pre_start and on_post_start made it easier to know in what order the events will happen.

When I look at command, working_directory, initialization_options. well it works. I cannot say that it is bad.

But I also think that it might be easiers to do
on_pre_start(context)
on_post_start(context, weaksession)

[predragnikolic]

Also I would suggest to keep the context always the first argument context, to have some consistecy

[rchl]

I'm also skeptical about command and intialization_options but having on_pre_start and on_post_start doesn't seem necessary and it doesn't really make it clear what is PRE and POST.

Also I would suggest to keep the context always the first argument context, to have some consistecy

This is actually out of date - context was removed from init.

[predragnikolic]

I have seen that on_execute_command now returns a Promise,
Which means that plguins now need to import a Promise.

I do think that a Promise is not necessary here...
And note, I dont mind the proposed API in this PR.
I just want to see if I can remove the need to import Promise.

Here are 2 similar ideas:
https://github.com/Istok-Mir/Mir-cspell/blob/d710561c5bcf5e22e28d73ee00c89db356e433e9/main.py#L66-L73

Lets say that we have a method:

def register_command(self, server_command, sublime_command_to_invoke):
    ...

we register commands before we start the server

def on_pre_start(self, context):
    self.register_command('someServer/command', 'hello')


class HelloCommand(LspTextCommand):
    session_name= 'server'
    def run(self, server_args):
        ...

and unregister the commands when we destroy the server.

[rchl]

I have seen that on_execute_command now returns a Promise,
Which means that plguins now need to import a Promise.

Promise is gonna be very useful and help with cleaning up plugins code IMO.

I do think that a Promise is not necessary here...

Not necessary, sure, but what is? :)
This replaces an awkard previous approach where there was a bool + callback.
Packages had to do some awkward code to handle that and it wasn't that readable. Promise is IMO much better.

Lets say that we have a method:

I've mentioned to you in private chat that I like the approach that Mir took but I don't think that we should mix two different approaches here.

If we want to switch everything (or at least all class method) to a single init method then it would make sense to register things like that using self.foo. But introducing that approach together with the current style doesn't feel right.

[rchl]

I've mentioned to you in private chat that I like the approach that Mir took but I don't think that we should mix two different approaches here.

I'm seriously considering adopting that approach here in some capacity.

Instead of additional_variables, install_async, command, initialization_options and working_directory we would have a single method like on_prepare (or something) where one would set needed things directly on the context.configuration. For additional_variables and working_directory we could expose those on PluginContext also.

Let me know before I start working on it if it sounds like a good idea. I think that that approach would be better because:

• it would be easier to add functionality in the future without breaking backwards compatibility
• the logic of the plugin wouldn't have to be spread across multiple methods. For example, currently install_async prepares server and has to store some info about it in class instance that additional_variables can access later. If everything is in one method then flow is easier to read and there is less code.

[predragnikolic]

Not necessary, sure, but what is? :)

Hopefully I can clarify the idea here with some mini POC

# first
# in LSP we would have _commands dict where we store registered commands
_commands: dict[str, str] = {}

def register_command(command: str, sublime_command: str):
    _commands[command] = sublime_command

def to_sublime_commands(command: str) -> str | None:
    return _commands.get(command, None)

# then
# In lsp where wae need to send the execture command to the langauge server 
sublime_command = to_sublime_commands(command)
if sublime_command: 
    self.view.run_command(sublime_command, {'arguments': arguments})
    # after handling the commands on the client side, don't send the command to the server
    return

server = server_for_view(server_name, self.view)
if server:
    params: ExecuteCommandParams = {"command": command}
    if arguments:
        params["arguments"] = arguments
    req = server.send.execute_command(params)
    _ = await req.result


# in the LSP-* we would register the command
register_command('cSpell.editText', 'cspell_edit_text')

class CspellEditTextCommand(LspTextCommand):
    def run(self, edit, arguments: EditTextArguments):
        _uri, document_version, text_edits = arguments
        apply_text_edits(text_edits).then(open_view)

Previously we returned a bool to know if the command was handled.
Now we return a Promise or None to know if the command was handled. Promise means that a LspPlugin handled the command.

The proposed idea was that we don't need to rely on a Promise to know if the command was handled.
Instead we can also know if a command "will be" handled if it was registered(key is present) in the _commands dict, and if the "command" is present in the _commands dict, that will mean that we will handle it on the client side, else the command will be send to the server

[rchl]

I get that but that's not a small improvement to the current API - it's a complete rewrite using different philosophy.

Don't get me wrong, I like this approach and would be willing to implement it that way but it would invalidate the whole work here. :)

Don't necessarily agree that it should be bound to a text command though. Execute command is not always related to a view.

[predragnikolic]

Let me know before I start working on it if it sounds like a good idea. I think that that approach would be better because:

For me it sounds like a good idea. But stop if you see that it is going in the wrong direction.

I can already work with the API design in this PR.
It solved some of the limitations of the previous approach.

The changes here are fine to me, but feel free to experiment with different approaches.

[predragnikolic]

it's a complete rewrite using different philosophy.

I send the previous message before GitHub showed me you last message. So yes, don't bother implementing, all fine

[predragnikolic]

There are some fine improvements in this PR.
This is just a first look at the PR, will try to take a look once more, tonight and tomorrow.

I did leave some wishes that I would like to see in the next version of LspPlugin.

Left some ideas just so you can consider them. Feel free to steal them if you like some of them, else feel free to discard them.

This is just an initial first review from my side.

[rchl]

It would be nice if the new API completely replaced lsp_utils, and made the plugins only rely on LSP.

Moving api decorators to LSP is a step in that direction. Also I guess a bit better and easier to use API through LspPlugin.

But I think it would be hard to convince everyone (maybe even myself) that things like NodeRuntime or UvVenvManager should be in LSP.

If we add file pattern support in the future
It would be nice to think in advance and see if the current API design would make it easy to add file patterns sometimes in the future.

I guess the only relevant method is is_applicable. I don't think we'd need to change anything in the API for that. Lets say we add opt-in activation_events in ClientConfig. I don't think that would affect the API in any way.

[predragnikolic]

Should the new API design also solve #2491 ?

EDIT: Yes, yes it does address that issue.

[predragnikolic]

Let say that we want to override project settings for a LSP-rust-analyzer

Currently we need to specify

{
    // folders: [
    //   ...
    // ]
    "settings": {
        "LSP": {
            "rust-analyzer": {
                "settings": {
                    //Setting-here
                }
            }
        }
    }
}

In the old API the key in project overrides, I think, Was the config name.
With the new API in this PR,
the key value that we need to specify in project overrides is still the name property? (Not the PackageName) Is that right?

[rchl]

With the new plugin API it would the package-name so it's a breaking change. It was discussed in #2739 (comment) but kinda left without final conclusion.