openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
581 stars 103 forks source link

How do you match a glob pattern at the top-level? #490

Closed bzoracler closed 2 months ago

bzoracler commented 2 months ago

I'm not sure if I'm configuring pygls wrong, or if it's the client's issue, or if this is a bug.

I followed #376 to dynamically register the feature workspace/didChangeWatchedFiles, with the watched file pattern **/*.py, which worked fine; 0-or-more nested directories when Python files are added/deleted/changed trigger the notification workspace/didChangeWatchedFiles.

Now, I'm trying to restrict the pattern to files at the top-level. Following the LSP specification, I assume this would look like (following a typescript file extension)

*.ts

but this isn't working ( workspace/didChangeWatchedFiles is not being triggered when adding/deleting/changing .ts files).

I've tried the following:

If any of this makes any difference:

tombh commented 2 months ago

I'm afraid I'm not very familiar with this area, but my first thought is that, as you suggest, this is more of a client issue. Which editor are you using?

bzoracler commented 2 months ago

I've tested this on 2 clients:

The dummy server implementation I'm using:

from pygls.server import LanguageServer
from lsprotocol import types

server = LanguageServer("example-server", "v0.1")

@server.feature(types.INITIALIZED)
def init(ls, params):
        ls.register_capability(types.RegistrationParams(
        registrations = [
            types.Registration(
                id = "matcher-test",
                method = types.WORKSPACE_DID_CHANGE_WATCHED_FILES,
                register_options = types.DidChangeWatchedFilesRegistrationOptions(watchers = [
                    types.FileSystemWatcher(glob_pattern = "*.ts", kind = types.WatchKind.Create | types.WatchKind.Change | types.WatchKind.Delete),
                    types.FileSystemWatcher(glob_pattern = "./*.js", kind = types.WatchKind.Create | types.WatchKind.Change | types.WatchKind.Delete),
                    types.FileSystemWatcher(glob_pattern = "**.ini", kind = types.WatchKind.Create | types.WatchKind.Change | types.WatchKind.Delete),
                    types.FileSystemWatcher(glob_pattern = "**/*.cfg", kind = types.WatchKind.Create | types.WatchKind.Change | types.WatchKind.Delete),
                ])
            )
        ]
    ))

if __name__ == "__main__":
    server.start_io()

In this example, only .cfg additions/changes/deletions trigger the notification workspace/didChangeWatchedFiles (but this triggers it at any nesting level from the root downwards). I can't get additions/changes/deletions of .ini, .ts, and .js to trigger anything at the root.

Given that it doesn't work in 2 different clients, I'm inclined to think that it's something on the server side.

bzoracler commented 2 months ago

The LSP specifications for workspace/didChangeWatchedFiles aren't very clear, but in the two clients I tested (pygls's VSCode extension and LSP4IJ), the glob patterns match absolutely, not relatively.

Therefore, providing a relative glob pattern for the top level, such as *.py, does not work. In contrast, providing an absolute pattern does. If the workspace project consisted solely of a setup.py laid out like the following,

  1. /home/<Name>/projects/my-project/<root>/setup.py (non-Windows)
  2. C:\Users\<Name>\projects\my-project\<root>\setup.py (Windows)

then passing an absolute glob pattern, with some path normalisation handled somewhere between the server and client, will work. The pygls VSCode extension should be able to pick up setup.py using the following patterns, without looking into workspace subdirectories:

  1. /home/<Name>/projects/my-project/<root>/*.py (Non-Windows - no changes)
  2. c:/Users/<Name>/projects/my-project/<root>/*.py (Windows - drive letter lower-cased, path separators converted to POSIX-style)
tombh commented 2 months ago

So, it is possible to achieve non-recursive, top-level file change notifications by using an absolute path (as opposed to a relative path)? And that's why you closed the issue?

Let me just summarise my understanding of using the glob_pattern argument, to see if we agree:

  1. Providing the traditional double asterisk wildcard path, **/*.py, does work as expected for recursive paths relative to the workspace.
  2. Providing an absolute path with a file-specific wildcard, /home/tombh/projects/foo/*.py, works as expected for non-recursive paths.
  3. Providing a plain file-specific wildcard path, *.py, does not work at all. My intuition for this is that, even without RelativePattern(), it should default to a non-recursive, workspace-root pattern. Therefore, what you were expecting to begin with, and why you made this issue. This could be because of ambiguity in the LSP spec (which we can address). Or at the very least Pygls can justifiably take advantage of the ambiguity and offer the sane default, therefore automatically prepending the workspace root so that clients at least return something rather than nothing.
  4. The above point 3, doesn't even work with RelativePattern(base_uri="/home/tombh/projects/foo"). I feel like this must be a bug in Pygls.
bzoracler commented 2 months ago

I agree with most of your summary - except whether pygls should provide a default for patterns like *.py in point 3, and whether point 4. is a bug. I didn't yet inspect how the clients interpret the data provided by pygls with a lsprotocol.types.RelativePattern (or even if they use it at all).


I closed this issue because I realised that you were right in it being a client issue, and I'm not sure if there's anything actionable in pygls. Absolute paths like /home/tombh/projects/foo/*.py are likely working because of an implementation detail by the client, not something described in the language server specification:

tombh commented 1 month ago

Sorry for the late reply. This thread is a good insight and summary of the issue, so I'm glad you brought it up.

So let's assume that there's nothing that Pygls can do to help at the moment. But we can revisit if needs be.