Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate lsp_utils #2388

Closed
wants to merge 1 commit into from
Closed

feat: integrate lsp_utils #2388

wants to merge 1 commit into from

Conversation

rchl
Copy link
Member

@rchl rchl commented Jan 3, 2024

Integrate lsp_utils into core since with PC4 we can no longer expose settings in dependencies so this allows us to integrate those into LSP settings.

It's a work in progress:

  • Still not sure about final directory structure. Currently exposed as LSP.api.*.
  • lsp_utils has been providing its own API docs. To make it possible I had to do a small patch in the LSP code base before running sphinx as otherwise it crashed. Here it would be problematic to do the same during local development as it would break LSP. But I guess I could clone LSP into a separate directory like I did in lsp_utils. But besides that, not sure where would that documentation live. Ideally it would be integrated into main LSP documentation but all solutions I've looked at are kinda janky. Maybe should check https://github.com/mkdocstrings/mkdocstrings

@jwortmann
Copy link
Member

How about to just add the settings to LSP, but leaving lsp_utils as a dependency (I don't really think the code belongs into LSP)? Then it could just simply load its settings from LSP.sublime-settings, but nothing else needs to change.


Offtopic:
I never understood why LSP-file-watcher-chokidar is a separate package instead of being integrated into LSP (perhaps disabled by default). If it was integrated, then it could be possible to add support for workspace/didCreateFiles, workspace/didDeleteFiles. etc. notifications.

@rchl
Copy link
Member Author

rchl commented Jan 15, 2024

How about to just add the settings to LSP, but leaving lsp_utils as a dependency (I don't really think the code belongs into LSP)? Then it could just simply load its settings from LSP.sublime-settings, but nothing else needs to change.

It would work I guess but why do you think it doesn't belong in LSP? What are you arguments?

When creating lsp_utils initially I also thought that but I'm not sure anymore. It's not like it's used by anything other than LSP and also it's used by majority of LSP packages (I approximate).

Actually the main goal of lsp_utils at the beginning was to provide compatibility layer for ST2 and ST3+ and that point is irrelevant now since the code was forked.

And also merging would provide easier way to migrate to PC3 (even if the issue with dependencies not being supported will get fixed at some point).

Offtopic:
I never understood why LSP-file-watcher-chokidar is a separate package instead of being integrated into LSP (perhaps disabled by default). If it was integrated, then it could be possible to add support for workspace/didCreateFiles, workspace/didDeleteFiles. etc. notifications.

If lsp_utils gets merged then I guess it could also since it depends on node instance from it.
But I doubt that it will actually help much with supporting those features as file watchers are not reliable enough to detect file creations/deletions (can easily get confused by file moves or branch changes).

@jwortmann
Copy link
Member

Maybe it's not that it wouldn't belong into LSP, but to me it looks like it would add a lot of unnecessary duplication and it doesn't seem to be integrated very well. In my opinion the following points would make things more complicated as they are now:

  • what is the point of having separate APIs, the existing one from LSP.plugin and then another new one at LSP.api? New classes and functions should just be added to the existing LSP.plugin API.
  • why are there so many subclasses GenericClientHandler < ClientHandler < AbstractPlugin exposed, so that if you write a plugin you don't know where to start and which one to use? Ideally there should be only a single one of those generic plugin classes. If the class is missing some useful methods, then just add them to that class.
  • there are many duplicated methods, but with different names, e.g. get_display_name() vs. name(), get_additional_variables() vs. additional_variables() etc. What is the point of having the duplicate names for the same things?
  • the default implementation of get_display_name() is in contradiction to the docstring of the base class:
    A human-friendly name. If your plugin is called "LSP-foobar", then this should return "foobar". If you also

So to me this looks like that a lot of it just introduces alternatives to do something that is already implemented. For the plugin classes, perhaps it would be better to just expose something like NpmPlugin and PipPlugin in addition to the existing AbstractPlugin. Otherwise we end up with different standards for the same thing, and that will be bad to maintain. Similar to the situation with Session.set_window_status_async() and Session.set_config_status_async() methods, which basically do the same thing (one of them just adds parentheses around the message). But maybe I just don't see what GenericClientHandler and ClientHandler are supposed to be used for?

@rchl
Copy link
Member Author

rchl commented Jan 17, 2024

Well, if we'd want to do any changes like that then we might as well just come up with completely new API since the current one is too limiting. Many methods, especially class methods, don't provide enough context so LSP packages have to use global functions to "guess" those.

@predragnikolic
Copy link
Member

I been thinking about this PR for a while,
what do you think about the following:

  1. Fix PC not supporting settings for dependencies.

I would suggest moving lsp_utils./lsp_utils.sublime-settings to LSP/lsp_utils.sublime-settings
Add the Preferences: LSP Utils Settings to the command palette.

That seems like the most simple way to make settings work for lsp_utils.
The small downside is that this file LSP/lsp_utils.sublime-settings will be there even for LSP-* packages that don't rely on lsp_utils.

  1. Improve the API for plugins or create a new one

why are there so many subclasses GenericClientHandler < ClientHandler < AbstractPlugin exposed... Ideally there should be only a single one of those generic plugin classes.

+1 for this.

All in all, @jwortmann had some really good comments.
if we merge lsp_utils, that means that we will also merge some unnecessary code.

I am more for change the API for plugins slowly in a new shape in LSP. (The precondition is that the new shape has to be more carefully throughout)

@rchl
Copy link
Member Author

rchl commented Jan 19, 2024

Sure. Done #2395 and will drop this and look into creating a new API.

@rchl rchl closed this Jan 19, 2024
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.

3 participants