-
Notifications
You must be signed in to change notification settings - Fork 409
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
merlin: add rules always, regardless the configuration of (merlin)
field
#10325
Conversation
b174a61
to
0b4f069
Compare
Signed-off-by: Javier Chávarri <[email protected]>
Signed-off-by: Javier Chávarri <[email protected]>
0b4f069
to
0c00e11
Compare
It's not great that everyone will have to pay for this performance penalty for this relatively niche feature. What about just adding a setting for enabling/disabling the merlin per context? |
It is worth noting that the penalty for users with a single Dune context (which I am guessing should be the majority) will be non existent.
The feature doesn't exist yet, so it definitely has very few users 😄
You mean some new field in context like (context
(default
(name foo)
(generate_merlin_rules false)
(merlin))) ?
Edit: I realized later that nothing would happen if |
I also searched for |
I implemented an interpretation of this suggestion in #10328. |
Signed-off-by: Javier Chávarri <[email protected]>
@rgrinberg Any thoughts on the above comments or the alt approach suggested that was implemented in #10328? Considering how niche the usage of multi-context is, I tend to think introducing this extra work will not have a lot of impact of users (as opposed to adding yet another configuration flag to Dune). cc @anmonteiro (Apologies for pinging, but I am blocked on either this PR or #10328 in order to continue the work for new commands being done in #10324). |
If we had a way to make cross compilation work without contexts, maybe this would be acceptable. Your usage search omits all the uses of opam-cross-{windows,android}, which use environment variables to setup a cross compilation build environment. Essentially all OCaml cross compilation is done through these repos, so it's pretty important. #10328 is the right way to go. |
This change is related to the introduction of selected context for merlin controlled via ocaml-lsp (see related issue ocamllabs/vscode-ocaml-platform#1432, and ongoing dune work in #10324).
The change consists on always adding merlin rules, regardless the user choice of context through the
(merlin)
field indune-workspace
.The reason for this change is that it unlocks dynamically switching the context used for merlin without the need to rebuild again, nor changing the
(merlin)
value indune-workspace
(which kind of defeats the goal of dynamically switching context).In theory, the change should not introduce any regressions, besides the expected extra time to create and execute the new rules added, because each context is independent from the others.
It shouldn't change any external behavior from the Dune user point of view either, because
Ocaml_merlin
isn't reading the information fromContext
but from the workspace value directly:dune/bin/ocaml/ocaml_merlin.ml
Line 138 in 9265e2c