-
Notifications
You must be signed in to change notification settings - Fork 74
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
Feature: Allow users to choose which Dune context to use #1432
Comments
I'm wondering if you've considered implementing this via command line arguments, and respawning the ocamllsp when context changes:
that way, when one changes context in editor, a new ocamllsp process will be spawned with different command line arguments this won't require patches to merlin as I understand |
@andreypopp Thanks for the suggestion, I didn't consider that. Yes, I think that would avoid touching merlin because essentially we would be sidestepping the additions to the protocol by moving them to the command line flag. Do you know if restarting the ocamllsp process often would have any downsides? (I understand the Also, I wonder if the suggestion in ocaml/dune#10324 (comment) would be somehow related or affected by this decision. If we ended up handling context switching (and merlin config) through Dune RPC, then I assume no patches to merlin would be necessary either. |
Maybe we shouldn't even kill/restart it on switch but keep the processes working? e.g. on start we start ocamllsp for the default context, once In the future, we can even consider showing diagnostics from several ocamllsp processes at once (though need to think through it, not to overwhelm a user with multiple similar errors).
No idea, but if we go the cli configuration way then I think By the way, if we are going to allow melange context to have a different compiler version than default context, then we definitely should start a separate ocamllsp/merlin instance for different contexts. As I understand now merlin works only with a single OCaml version once it is installed into the switch. |
I understand
Good point. I wasn't considering this case at all, just because Ahrefs use case is within a single opam switch. But considering Dune contexts can exist on different switches, that looks like the way to go. |
Following the discussion in ocaml/dune#10324 , adding a
That's correct. |
After some discussions with @andreypopp, and with vscode-ocaml users at Ahrefs (@davesnx), we are evaluating alternative implementations to the solution proposed in #1449. The main limitation of the context selection in #1449 is that it is global and only one context (LSP server) can be active at any given time. So in a project where two or more contexts exist, and libraries or executables can be defined on one or the other, global selection is problematic because there will always be files in the project that don't belong to the chosen context. These files will show broken errors when the selected context is not the right one, which is quite a bad experience. To mitigate this, we could go with an alternate solution, where users can define multiple language server configurations and assign them to specific folders in the workspace. This could be implemented as an extension of the existing {
"ocaml.server.args": [
{
"dir": "frontend",
"args": ["--context", "melange"]
},
{
"dir": "backend",
"args": ["--context", "default"]
}
]
} Then, when opening a document, the extension would:
let client_options () =
let documentSelector =
LanguageClient.DocumentSelector.
[| language ~pattern:"melange/**/*" "ocaml"
; language ~pattern:"melange/**/*" "ocaml.interface"
; language ~pattern:"melange/**/*" "ocaml.ocamllex"
; language ~pattern:"melange/**/*" "ocaml.menhir"
; language ~pattern:"melange/**/*" "reason"
|] in ... This way, we could also remove the need for a specific Would this be a better approach and is |
Are there any other server arguments that would have to change for each directory? If not, something like this could be simpler: {
"ocaml.server.contexts": {
"frontend": "melange",
"backend": "default"
}
} |
Thanks for the input @mnxn. After evaluating more carefully the folder layouts we have internally, I am thinking the solution I was suggesting (either with Besides, the changes to the extension to support multiple LSP clients + servers seems a bit convoluted too. Instead, I think for a first version we can let users choose context using one of these options: a) Single workspace: switch context using the "settings": {
"ocaml.dune.context": "melange"
} |
We are currently experimenting with separate Melange and OCaml libraries and place them in different Dune contexts, in order to improve the developer experience (see melange-re/melange#694).
Under this multi-context setup, it would be useful as a user to select which Dune context to use in VSCode for code definitions, types and errors.
We are putting out this proposal to gather feedback from maintainers, and if accepted, make sure the implementation plan is ok.
Current status
It is currently possible to choose the context by editing
dune-workspace
files and adding(merlin)
to the context we want to work on, but this has a few downsides:dune-workspace
every time one wants to switch contextocaml-merlin
won't pick up the new context straight away, so it shows errors like:No config found for file foo.ml. Try calling 'dune build'
until one restarts the whole VSCode windowProposal
To improve the experience, the proposal is to add a new command
select-context
to this extension, which will show users a list of contexts to choose from, and record their choice in vscode settings (similar to the current experience with sandbox selection inselect-sandbox
).Implementation
For this to happen, besides the new VSCode command
select-context
, we will have to add a few other pieces in ocaml-lsp-server and Dune'socaml-merlin
:ocaml-lsp-server
ocamllsp/getDuneContexts
to the lsp server. This request will resolve to callingocaml-merlin
behind the scenes (see below)didChangeConfiguration
for a newly addedduneContext
config. This handling will forward the change toocaml-merlin
(see below)ocaml-merlin
GetContexts
that returns a list of available contexts in the current workspace, so that they can be sent throughocamllsp/getDuneContexts
and shown in a list inside VSCode extension UISetContext
that allows to define the current context, which will be called from ocaml-lsp-serverdidChangeConfiguration
handlerDoes the above plan make sense? Thanks!
The text was updated successfully, but these errors were encountered: