-
Notifications
You must be signed in to change notification settings - Fork 184
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
Conversation
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 Offtopic: |
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).
If lsp_utils gets merged then I guess it could also since it depends on node instance from it. |
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:
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 |
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. |
I been thinking about this PR for a while,
I would suggest moving That seems like the most simple way to make settings work for
+1 for this. All in all, @jwortmann had some really good comments. 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) |
Sure. Done #2395 and will drop this and look into creating a new API. |
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:
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 inlsp_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