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

Notify LSP of workspace config when starting #1321

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

EduardoRFS
Copy link
Contributor

Context

Currently the extension doesn't notify the LSP of configurations, this means that since ocaml/ocaml-lsp#1134 codelens are disabled whenever you start VSCode, even if you explicitly opt-in to codelens.

This is also true for disabling dune diagnostics like #1320.

Warning

This PR enables codelens again by default, as they're enabled in the manifest of the extension, I feel like this may be desirable anyway so I didn't change it, but if the extension wants to disable it by default, is just a matter of changing this https://github.com/ocamllabs/vscode-ocaml-platform/blob/master/package.json#L267

Related

@EduardoRFS
Copy link
Contributor Author

It seems like this is the old method of sending configurations.

This pull model replaces the old push model were the client signaled configuration change via an event. If the server still needs to react to configuration changes (since the server caches the result of workspace/configuration requests) the server should register for an empty configuration change using the following registration pattern:

It's likely that supporting this is a good idea as old LSPs are still gonna be working.

@voodoos
Copy link
Collaborator

voodoos commented Jan 5, 2024

Could you split the PR in two parts ? First the "send config on start fix" and then the new configuration option.

Thanks

@EduardoRFS EduardoRFS force-pushed the push-config-on-start branch 2 times, most recently from c5eca0b to 7b25341 Compare January 5, 2024 14:39
@EduardoRFS
Copy link
Contributor Author

Done, also pushed changelog

@EduardoRFS
Copy link
Contributor Author

@smorimoto as you're already here can we get this reviewed and hopefully merged?

@smorimoto
Copy link
Collaborator

Let me just take a look at this! 🙂

@smorimoto
Copy link
Collaborator

At least to me this looks a right fix.

@smorimoto smorimoto merged commit 4657b84 into ocamllabs:master Jan 5, 2024
7 checks passed
@rgrinberg
Copy link
Contributor

@mnxn or @ulugbekna could you review this?

@@ -2,7 +2,7 @@ open Import

type t

val make : unit -> t
val make : ?codelens:bool -> ?extended_hover:bool -> unit -> t
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use optional arguments unless there's a specific reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, was just copying the pattern from somewhere else in the codebase.

LanguageClient.OcamllspSettings.create

But now that I think about it, this is a binding and that's likely the reason why it is using optional parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

LanguageClient.OcamllspSettings.create

That one is weird as well. It's our own private API, what's it doing inside the vscode API bindings?

But now that I think about it, this is a binding and that's likely the reason why it is using optional parameters.

What do you mean by "binding"?

@@ -23,6 +23,9 @@ val ocaml_version_exn : t -> Ocaml_version.t

val start_language_server : t -> unit Promise.t

val set_configuration :
t -> ?codelens:bool -> ?extended_hover:bool -> unit -> unit
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@EduardoRFS EduardoRFS deleted the push-config-on-start branch January 5, 2024 18:15
@mnxn
Copy link
Collaborator

mnxn commented Jan 7, 2024

mnxn or ulugbekna could you review this?

Logic looks fine.
Style-wise, the Option.map + OcamllspSettingEnable.create code in send_configuration is a bit repetitive since it's repeated for each parameter. Especially since the master branch now has a third setting from #1320. Perhaps it could be abstracted into a helper function in a future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants