Support --context flag of dune ocaml-merlin
#1238
Conversation
jchavarri
force-pushed
the
dune-merlin/add-getsetcontexts
branch
from
3eea746
to
530c182
Compare
jchavarri
force-pushed
the
dune-merlin/add-getsetcontexts
branch
from
cdefa22
to
1a778fb
Compare
jchavarri
changed the title
getDuneContexts method and spawn a new ocaml-merlin process per Dune context
jchavarri
force-pushed
the
dune-merlin/add-getsetcontexts
branch
from
444095b
to
6ed35fe
Compare
jchavarri
force-pushed
the
dune-merlin/add-getsetcontexts
branch
from
6ed35fe
to
ba7b147
Compare
jchavarri
changed the title
getDuneContexts method and spawn a new ocaml-merlin process per Dune contextgetDuneContexts method and --context flag
| ()) | ||
| | Some dune -> ( | ||
| let describe = | ||
| "DUNE_CONFIG__GLOBAL_LOCK=disabled " ^ Fpath.to_string dune |
There was a problem hiding this comment.
DUNE_CONFIG__GLOBAL_LOCK=disabled
^ :(
but the question is — is dune describe contexts command's output stable?
There was a problem hiding this comment.
I see the need for DUNE_CONFIG__GLOBAL_LOCK as a bug / limitation of Dune, rather than a hack in this PR. The main issue is that Dune doesn't differentiate between commands that write in _build vs commands that only read. So it will always lock the folder regardless. I opened an issue to discuss making the lock mechanism smarter: ocaml/dune#10430
but the question is — is
dune describe contextscommand's output stable?
This subcommand is being added in the companion PR ocaml/dune#10324. It only outputs the names of the available contexts, so it shouldn't need to change over time.
Co-authored-by: Andrey Popp <[email protected]>
|
I talked to @andreypopp and he suggested to remove the |
Co-authored-by: Andrey Popp <[email protected]>
andreypopp
changed the title
getDuneContexts method and --context flag--context flag of dune ocaml-merlin
|
Thanks for the reviews @andreypopp. From my side, there's nothing else to add to this PR. Also, the parent PR in dune ocaml/dune#10324 has been merged, and it should be included in next release 3.16. |
|
@jchavarri is it the responsibility of the editor plugin to not use that flag if the installed dune version does not understand it ( |
| @@ -8,6 +8,8 @@ | |||
|
|
|||
| - Support folding of `ifthenelse` expressions (#1031) | |||
|
|
|||
| - Add `--context` flag (#1238) | |||
There was a problem hiding this comment.
I think we could add a bit more context here, at least define what "context" refers to here.
|
@jchavarri is this PR ready ? What will happen if the installed dune does not understand that flag ? |
|
I don't think this is needed anymore. Turns out that contexts alone aren't good enough for melange support. |
|
@rgrinberg Should ocaml/dune#10324 be reverted? That PR was added exclusively to support this use case. If context selection is not useful for any other use cases besides melange, then it probably doesn't make much sense to keep the code added in that PR. |
|
Yes, there's probably no point to it. Though I think we should focus on implementing the new feature and what would that look like. We can clean things up in the end. |
This PR integrates two new Dune commands into the LSP server. See related issue vscode-ocaml-platform#1432.
Dependencies and related PRs
This PR relies on features being added in dune#10324.
It also unblocks the feature implemented in vscode-ocaml-platform#1449. A screen capture demo is shown in that issue.
Description
ocamllsp/getDuneContextsthat will return the list of available Dune contexts in the workspace (through the newly addeddune describe contextssubcommand)--contextin the server. This flag, when present, will be passed through to thedune ocaml-merlininstance being created.