Skip to content

Conversation

@jwortmann
Copy link
Member

@jwortmann jwortmann commented Dec 10, 2025

Combine selector and should_ignore plugin API methods into a new is_applicable.

The previous methods are intended to still work with this PR, i.e. it should be backwards compatible (todo: still needs testing to verify).

This also adds a new TextCommand which can be used via

view.run_command('lsp_check_applicable', {'session_name': 'LSP-foo'})

to retrigger the check for the given view and session. This is implemented as a command (instead of additional API method) to allow it to be called even if the session is not yet running (this needs it to be called e.g. from an independent listener implementation of course).

Closes #2503

@jwortmann
Copy link
Member Author

Language servers can dynamically register features only to certain files using a DocumentSelector, which is a list of DocumentFilter, which in turn is a combination of language ID, scheme and glob pattern. We use that DocumentSelector to assing the server capabilities to the SessionBuffers, but the criterium which views the language server runs on is currently still limited to the schemes and selector setting from the config (settings file).

The new is_applicable method should make it possible/easier to match the files supported by the language server. For example a server could provide some features for a Project.toml (or Cargo.toml, etc.) configuration file, but should not run on any other TOML files. There might be awkward ways to achieve that using the previous API methods or by creating a special syntax only for such filenames and using a special scope name. But it should be much easier now using something like

@classmethod
def is_applicable(cls, view: sublime.View, config: ClientConfig) -> bool:
    if (filename := view.file_name()) and os.path.basename(filename) == 'Project.toml':
        return True
    return super().is_applicable(view, config)

jwortmann and others added 2 commits December 11, 2025 12:19
Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>
@rchl
Copy link
Member

rchl commented Dec 11, 2025

I haven't given it too much thought but would it make sense to also (or only) provide a way to statically define those conditions in settings (to match the current approach)? It might not be as flexible as API-based solution but it might be enough if one can specify equivalent of DocumentFilter that way.

Something like:

document_filter: list[DocumentFilter]

but without language

The advantage of that? A clear visibility of the conditions triggering the server.

EDIT: Copilot wants to read ignore patterns from .copilotignore file so that should also work for it if it would read those and store in config settings before starting server. And it would make its implementation simpler as it wouldn't need to match globs itself.

@jwortmann
Copy link
Member Author

I think providing an API method that can only modify the static config would be a step backwards because then there wouldn't be any way to react to changes in configuration files when the server is already running. The is_applicable just combines the existing methods while giving more flexibility (for instance the previous should_ignore could only be used as an additional condition when the static config already matches, but not to allow more files to be applicable) and with a better name.

We could think of adding something like the DocumentSelector to the static config independently of that API method and at a later time. Then we just need to modify the default implementation of is_applicable and the corresponding code for other configs that are not backed by a plugin. If I had started creating the LSP package, maybe I would have done it that way to keep the implementation close to what is defined in the LSP specs and to how language servers work. But in that case I had probably also included the language ID instead of our selector, and then map the language IDs to scope selectors internally. But I think that ship has sailed long ago. If we add a new config option document_selector now, to allow a list of language/scheme/pattern combinations, it might be confusing how it should behave in combination with the existing selector and schemes options though (should those simply be ignored if document_selector is defined?).

Comment on lines +256 to +259
if wm._new_session:
wm._sessions.add(wm._new_session)
listener.on_session_initialized_async(wm._new_session)
wm._new_session = None
Copy link
Member Author

@jwortmann jwortmann Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is pretty ugly, but the public WindowManager.start_async method is not actually sufficient to start a new session. That method sets an internal variable at

self._new_session = session
and it expects some more code to be run after that, in order to properly handle the start of a new session.

I won't touch the recursive

def _dequeue_listener_async(self) -> None:
method because it is a riddle to me how that works...

Starting a new session when triggered from the lsp_check_applicable TextCommand seems to work correctly now though.

@jwortmann jwortmann marked this pull request as ready for review December 15, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

not possible to un-ignore a file after ignoring through should_ignore

2 participants