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]

You should avoid force-push when there are reviewers involved since that makes it harder to see what has changed.

In the end, when ready to merge, there is an option to squash and merge so the history will get cleaned up.

[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 ❤️

[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 sublimelsp/LSP#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.