Documentation update about dynamic features

[fda-odoo]

Hello !

I'm trying to use the WORKSPACE_DID_CHANGE_WATCHED_FILES notification. But I can't pass a DidChangeWatchedFilesRegistrationOptions on the decorator.

I already succeeded to pass a RegistrationOption for the RENAME_FILES event, like this:

@server.feature(WORKSPACE_DID_RENAME_FILES, FileOperationRegistrationOptions(filters = [
    FileOperationFilter(pattern = FileOperationPattern(glob = "**"))
]))
def did_rename_files(ls, params):
    """Workspace did rename files notification."""
    for f in params.files:
        #my stuff

But if I try to pass a registrationOptions for WORKSPACE_DID_CHANGE_WATCHED_FILES, it crashes:

@server.feature(WORKSPACE_DID_CHANGE_WATCHED_FILES, DidChangeWatchedFilesRegistrationOptions(watchers = [
    FileSystemWatcher(glob_pattern = "**/*.py", kind = WatchKind.Create | WatchKind.Change | WatchKind.Delete)
]))
def did_change_watched_files(ls, params: DidChangeWatchedFilesParams):
    """Workspace did change watched files notification."""
    print("Workspace did change watched files")

With this code I get a "MethodTypeNotRegisteredError". From what I see from the code, it seems to try to find a DidChangeWatchedFilesOptions class, which doesn't exist.
Am I doing something wrong or is there an issue with this option?

[fda-odoo]

I finally managed to make it work with this code:

@server.feature(INITIALIZED)
def init(ls, params):
    ls.register_capability(RegistrationParams(
        registrations = [
            Registration(
                id = str(uuid.uuid4()),
                method = WORKSPACE_DID_CHANGE_WATCHED_FILES,
                register_options = DidChangeWatchedFilesRegistrationOptions(watchers = [
                    FileSystemWatcher(glob_pattern = "**", kind = WatchKind.Create | WatchKind.Change | WatchKind.Delete)
                ])
            )
        ]
    ))

It could maybe be nice to indicate how we should register about dynamic features in the documentation. As you can see, I expected dynamic registration to be done this way too, as nothing (or I miss it) indicates it.

I think it's quite an interesting addition, as Microsoft is now saying that registering to WORKSPACE_DID_CHANGE_WATCHED_FILES is now a good practice (before no registration was needed).
But in practice it is more than a good practice as no event is fired if we didn't registered first !

see https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles (especially: "It is recommended that servers register for these file system events using the registration mechanism. In former implementations clients pushed file events without the server actively asking for it.")

[alcarney]

I finally managed to make it work

Glad to hear it!

I'll admit I'm not that familiar with this part of the spec, is this a more generic version of the workspace/did(Create|Rename|Delete)Files set of notifications?

It could maybe be nice to indicate how we should register about dynamic features in the documentation.

I agree! I'm kind of surprised we didn't have that already 😅

[fda-odoo]

This is not really a subset but it is events that are coming from different sources.
worskpace/did(Create|Rename|Delete)Files are notifications that are sent when the event is initiated by the user in vscode.
workspace/didChangeWatchedFiles are events that are launched by the FileSystem Watching. So vscode is unaware of the content of the change, but send the event of creation/deletion/update when he got it. You can see in the event that except the uri of the file, there is nothing else, because it is not originated by vscode.

To give you an example, I wanted to use this event to detect that git has changed the content of the workspace.
If you change your branch, or pull some code, even if you do it in vscode it doesn't trigger a FileChange, but instead the FileSystemWatcher.

However this is the first dynamic feature I register and I don't know if the "INITIALIZED" feature is the best place to do it, but it seems to work

[alcarney]

Ah that makes sense - thanks!

However this is the first dynamic feature I register and I don't know if the "INITIALIZED" feature is the best place to do it, but it seems to work

Well, you've officially registered more dynamic features than I have! It's not an area of LSP/pygls I've had much reason to look into yet.

INITIALIZED feels as good a place as any to me, getting it to work is the main thing :)

[tombh]

Thanks for bringing this up. Like @alcarney I'm not very familiar with this area either. We could add a note on the docs page? The interesting thing about the worskpace/did(Create|Rename|Delete)Files features is that they're Pygls built-ins, maybe workspace/didChangeWatchedFiles could be as well?

https://pygls.readthedocs.io/en/latest/pages/advanced_usage.html#built-in-features

[fda-odoo]

I don't really know what you want to be built-in with this feature, because it does not really do something. This is only an event that indicates that a file changed and give the uri, but not the content not anything else.

however I think it could maybe be interesting to check more generaly what can be done about dynamic features and the way they are registered. At first when I discovered pygls, I understood that as soon as you put a decorator @feature, pygls will handle the registration. But it is not doing it for dynamic features. So what about not implement a reaction to this event, but rather to automatically call the registration if you see a @feature('didChangeWatchedFiles') ?

[fda-odoo]

To continue on this subject, I just found this section: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration

Now you do not receive any 'DidChangeConfiguration' notification anymore, except if you dynamically register for it.
I can do it like earlier in this thread with:

@server.feature(INITIALIZED)
@send_error_on_traceback
def init(ls, params):
    server.register_capability(RegistrationParams(
        registrations = [
            Registration(
                id = str(uuid.uuid4()),
                method = WORKSPACE_DID_CHANGE_WATCHED_FILES,
                register_options = DidChangeWatchedFilesRegistrationOptions(watchers = [
                    FileSystemWatcher(glob_pattern = "**", kind = WatchKind.Create | WatchKind.Change | WatchKind.Delete)
                ])
            ),
            Registration(
                id = str(uuid.uuid4()),
                method = WORKSPACE_DID_CHANGE_CONFIGURATION,
                register_options = DidChangeConfigurationRegistrationOptions(
                    section = "???"
                )
            ),
        ]
    ))

However as the documentation says, to register you have to register to 'undefined'. The 'DidChangeConfigurationRegistrationOptions' is then useless, and after experimentation, is even not working if used.
The right registration is then

Registration(
                id = str(uuid.uuid4()),
                method = WORKSPACE_DID_CHANGE_CONFIGURATION
            ),

On the feature, the parameter is not filled anymore, you have to pull the the configuration now.

@server.feature(WORKSPACE_DID_CHANGE_CONFIGURATION)
def did_change_configuration(server, params: DidChangeConfigurationParams):
   #params is None, you have to do server.get_configuration
   pass

The weird thing here is that params is None, but if you don't put it in the signature the feature does not work at all.
I rename the issue too to match the real subject

[alcarney]

I'm finding this thread very thought provoking! Rereading the spec, there's a chance even pygls' static registration is currently too restrictive 🤔. - Does anyone know of a server that statically registers lsp methods with a DocumentSelector??

but rather to automatically call the registration if you see a @feature('didChangeWatchedFiles')?

It would certainly be possible for pygls to have a built in version of your INITIALIZED handler, though it might take a little bit of thought to get it right, especially since it appears that all the standard methods can also be dynamically registered.

I suspect though that servers calling server.register_capability() themselves will still be needed in some situations, so it would still be worth writing that up.

I don't have much to add on the WORKSPACE_DID_CHANGE_CONFIGURATION stuff, other than it could be fairly dependent on your client's implementation - e.g. I think (but don't know for sure) that neovim will send a notification whether you ask for it or not. Configuration in general seems rather complex

The weird thing here is that params is None, but if you don't put it in the signature the feature does not work at all.

It might seem odd, but for requests like shutdown require an explicit None to be sent, otherwise they're considered invalid.