-
Notifications
You must be signed in to change notification settings - Fork 33
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
Optional automatic upgrade for new client config features in existing user-defined language server definitions #791
Comments
I have not strong opinion about that since I don't know if user defined ls is used by a lot of people. If you already implemented this feature and you are confident, I am not against to have this feature. However I think it misses an important information in the user defined ls definition: |
That's interesting. I would have expected it to be the primary way that end users are using LSP4IJ. Is the main usage by folks extending LSP4IJ for their own language servers which are then used by end users?
Yes, good point. I do think it also makes sense to capture the language server template ID -- which currently seems to be present in the But, having said that, if you think this isn't going to be terribly valuable for general users of LSP4IJ, let's not do it. As I mentioned, I've already implemented this upgrade logic in my own plugin where I know the language server template/definition relationships concretely. I just thought it might be more useful for the broader set of LSP4IJ users than it sounds like it actually would be. |
Being a user of the user-defined LS myself, I might be biased - but I think the user-defined LS captures the advantages of the LSP very well (write one compliant language server, connect it to multiple clients/editors), and the most recent updates (JSON validation for the user-defined LS) were incredibly helpful. LSP4IJ's generic language client/server interface is among the most powerful ones I've come across so far (especially due to the ability to specify file patterns), so thank you guys very much for all the work! On the flip-side, since the user-defined LS works so well, and the features/documentation support is so self-explanatory, it's hard to gauge how much it's actually being used just from activity on this repository, because hardly anyone's going to report issues; I think the majority of issues still come from people writing IntelliJ plugins. I would definitely welcome the client configuration features proposed here. |
Thanks for providing the perspective of an end user of user-defined language servers, @bzoracler. To be clear, the client configuration features are absolutely available to you regardless of whether or not the upgrade logic described here is implemented. Without it you would just need to keep tabs on what new features are exposed through client config with each release of LSP4IJ and flag them on as desired (and/or supported by your language server(s)). As of 0.9.0, there are only two features exposed by LSP4IJ, |
Thanks so much @bzoracler for your nicely feedback! Do you think it could be possible to add a review on LSP4IJ Jetbrains marketplace by highlighting user defined ls like you did to promote this feature? |
Quite a few new LSP4IJ features are being added via client configuration, but the discoverability of those features for existing users is quite low because the defaults for these features generally favor backward-compatibility unless explicitly configured/enabled. While the preferred enablement defaults are being added to the respective bundled language server definition templates, users with existing language server definitions do not benefit from these changes unless users know to update the client config JSON. This will continue to be a growing issue as more and more new features are added via client config.
In my own plugin that uses LSP4IJ to improve the editor experience for JavaScript/TypeScript/CSS in Community Edition IDEs, I've implemented automatic upgrade logic to add newly-defined client configuration properties to existing language server definitions. This upgrade logic is very careful not to change any explicit end user client configuration. Basically it does the following:
Map
.Map
.In this way, if a client configuration property is added to an active client configuration and the user later changes its value or even removes it from client configuation, it will not be updated again because it will have been denoted as added previously.
In my plugin, this happens unconditionally, but if added to LSP4IJ, it would make sense for this to be conditional. This would require the addition of two new fields to the persisted
UserDefinedLanguageServerDefinition
configuration:clientConfigurationUpgradePolicy
- determines whether/how automatic upgrade of client configuration is performed for the language server; values arePROMPT
,ALWAYS
, andNEVER
; defaults toPROMPT
, but can be easily switched toALWAYS
orNEVER
via a Don't ask again checkbox in the prompt dialog or in the user-defined language server definition panel.clientConfigurationUpgradedPropertyNames
- the set of fully-qualified previously-upgraded client configuration property names; this is the "memory".With these changes, before LSP4IJ starts a language server based on a user-defined language server definition, it would do the following:
clientConfigurationUpgradePolicy == NEVER
, it just starts the language server.clientConfigurationUpgradePolicy == ALWAYS
, it applies them, notifies the user of the exact properties that were added, and starts the language server.clientConfigurationUpgradePolicy == PROMPT
, it prompts the user as to which should be applied, applies any that were selected and notifies the user of the exact properties that were added, and starts the language server. Note that all missing properties are added to the memory whether or not they were added to avoid repeated prompts for properties that were not selected.Again, aside from the small amount of work to make it optional, this is something I've already implemented in my own plugin, so it would really just be a port of something that already exists.
Let me know if you think this would be valuable or not for all LSP4IJ users and, if so, I'll be happy to implement it and get a PR going.
The text was updated successfully, but these errors were encountered: