feat: add decorators for api handlers

[jfcherng]

Just sharing an idea I got during playing with a mysterious Python language server + vs_marketplace_client_handler.

---

With this PR, you can register message handlers with decorators.

For example,

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

    # sometimes a handler handles multiple message methods...
    api.on_notification('intelephense/IndexingStarted', self.handle_indexing)
    api.on_notification('intelephense/IndexingEnded', self.handle_indexing)

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

def handle_open_doc(self, params, respond) -> None:
    ...

def handle_indexing(self, params) -> None:
    ...

can be written as

from lsp_utils import notification_handler, request_handler

@notification_handler("eslint/status")
def handle_status(self, params) -> None:
    ...

@request_handler("eslint/openDoc")
def handle_open_doc(self, params, respond) -> None:
    ...

@notification_handler(["intelephense/IndexingStarted", "intelephense/IndexingEnded"])
def handle_indexing(self, params) -> None:
    ...

[rchl]

Interesting

Pros:

• pretty nice and clean syntax
• avoids the "register" boilerplate

Cons:

• adds complexity in the code
• does not necessarily address any issue or annoyance since the current solution is already pretty OK IMO (certainly better than the magic methods in the AbstractPlugin, no offense ;)).

Also, I wonder whether it is types-friendly (haven't tried it myself)? Will it complain if the decorated function doesn't match the expected signature?

[rchl]

@as_notification_handler("eslint/status")
def handle_status(self, params) -> None:
    pass

You would have to import the as_notification_handler decorator first, right? How would you do that?

[jfcherng]

@as_notification_handler("eslint/status")
def handle_status(self, params) -> None:
    pass

You would have to import the as_notification_handler decorator first, right? How would you do that?

Ah, yes, the ApiWrapper has to be exposed in the __init__.py (or put those decorators in somewhere else publicly).

if lsp_version >= (1, 0, 0):  # type: ignore
    # ST 4
    from .vs_marketplace_client_handler_v2 import ApiWrapper  # <-----------------------------
    from .vs_marketplace_client_handler_v2 import VsMarketplaceClientHandler
else:
    # ST 3
    from .vs_marketplace_client_handler import ApiWrapper  # <--------------------------------
    from .vs_marketplace_client_handler import VsMarketplaceClientHandler

__all__ = [
    "ApiWrapper",  # <------------------------------------------------------------------------
    "ApiWrapperInterface",
    "ServerVsMarketplaceResource",
    "VsMarketplaceClientHandler",
]

And then you can use it like

from lsp_utils import ApiWrapper

@ApiWrapper.as_notification_handler("eslint/status")
def handle_status(self, params) -> None:
    pass

[jfcherng]

It resolves nothing in practice. Somehow feel it can be useful in some ways (not necessary in this PR).

[rchl]

Ah, yes, the ApiWrapper has to be exposed in the __init__.py (or put those decorators something else publicly). Something like

Conceptually that would be wrong as it's an implementation-specific class that is not supposed to be exposed.

[jfcherng]

Also, I wonder whether it is types-friendly (haven't tried it myself)? Will it complain if the decorated function doesn't match the expected signature?

I think yes. On my side, LSP-py* can complain after I make the function signature clear.

T_NOTIFICATION_HANDLER = Callable[["VsMarketplaceClientHandler", Dict[str, Any]], None]
#                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the first arg is always `self`

and

    @staticmethod
    def as_notification_handler(event_name: str) -> Callable[[T_NOTIFICATION_HANDLER], T_NOTIFICATION_HANDLER]:
        """ Marks the decorated function as a handler for the notification event. """

        def decorator(func: T_NOTIFICATION_HANDLER) -> T_NOTIFICATION_HANDLER:
            setattr(func, ApiWrapper.HANDLER_MARK_NOTIFICATION, event_name)
            return func

        return decorator

[jfcherng]

Ah, yes, the ApiWrapper has to be exposed in the __init__.py (or put those decorators something else publicly). Something like

Conceptually that would be wrong as it's an implementation-specific class that is not supposed to be exposed.

Or probably put decorators under NpmClientHandler, and use @self.as_notification_handler(...).

---

Oh no, self is not defined in that context. It has to be verbose @NpmClientHandler.as_notification_handler(...) :(

Or put them in something like npm_client_decorator.py... (feel stupid)

[rchl]

I'd also like this to be documented in new docs. I can handle that.

[jfcherng]

MUCH appreciated for PR reviewing (many lessons learned) and documenting. And that lsp_utils refactoring :heart:

[jfcherng]

The link to documents in the README is wrong btw.

[predragnikolic]

What do you think of a shorter name for the decorators?

@request_handler -> @on_request @notification_handler -> @on_notification

[rchl]

What do you think of a shorter name for the decorators?

@request_handler -> @on_request @notification_handler -> @on_notification

I feel that the existing naming is more in line with conventions I'm familiar with where decorator describes what it turns the function into (like @classmethod). on_ prefix would kinda suggest that it listens to something on the function it decorates.

[predragnikolic]

where decorator describes what it turns the function into

When you put it on that way it makes sense to name it request_handler notification_handler.

Here is the reason on why I suggested the name change. Because the @notification_handler does the same thing the same thing api.on_notification, I felt like that they should be named the same.

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

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

# the above achieves the same result as the bellow

@on_notification("eslint/status")
def handle_status(self, params) -> None:
    ...

the @on_notification decorator for me would suggest that the function it decorates will be executed when the notification is received.

[rchl]

I said:

on_ prefix would kinda suggest that it listens to something on the function it decorates.

and I think the example you made:

api.on_notification('eslint/status', self.handle_status)

is also in line with that statement. In the case of a central api object, we are adding a listener to that object so I think the names being different in those cases is appropriate. It's a different way of setting up a handler.

[jfcherng]

Can't wait for the next release. So many improvements for LSP-* devs :D

[jfcherng]

Currently the decorator methods can't handle some special notifications like $/progress, window/logMessage. Maybe we need to refactor the implementation of this PR, to make use of https://github.com/sublimelsp/LSP/pull/2496.

[rchl]

I feel like it was really only meant to handle custom requests/notifications. I think that it's consistent in that case?

It would be a bit ambiguous to specify what should happen if both LSP and a plugin handle given request at least.

[jfcherng]

It would be a bit ambiguous to specify what should happen if both LSP and a plugin handle given request at least.

If the client wants to make it into a mess, that's its own issue. I can't prevent that from happening if that's what the plugin author wants, but I need that ability to do something good.