Add flag to disable dune diagnostics #1221
Conversation
EduardoRFS
force-pushed
the
5.2-no-dune
branch
from
198eee1
to
585dffe
Compare
|
Should this use the workspace configuration settings ( like this https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeConfiguration )? Then you should be able to toggle it as well while you're editing (and goes with the rest of the configuration of the LSP) |
|
@tjdevries I thought about it, but I was not sure if OCaml LSP already had any options, but after checking it again, that seems to be the case, such as "extendedHover" and VSCode OCaml Platform even exposes that. And it even already supports the on change, the only downside is that I need to implement that, but will do, also keeping the diagnostics_loop running is likely better. |
EduardoRFS
force-pushed
the
5.2-no-dune
branch
4 times, most recently
from
ebebf08
to
5e87542
Compare
|
This patch works, but it shines light in a likely bug, ocaml-lsp doesn't request the configs when starting the LSP, this means that it only detects the settings when changing them. This seems to also explain why sometimes codelens is not loaded as the default is https://github.com/EduardoRFS/vscode-ocaml-platform/tree/no-dune |
|
Maybe another option would be to invalidate the diagnostics as soon as the buffer is edited ? (Not sure if that's allowed by the protocol however). |
|
@voodoos I think this would be possible, but in general seems weird, if you're changing the file is likely that you're trying to solves some of those errors, removing the errors would make that harder for developers on a single screen. |
|
I think the weird behavior of not pushing config was an issue with vscode-ocaml-platform and even if it is not, this patch seems to be safer overall. |
Another way to think about it is that most compiler errors for the currently edited buffer are redundant with the ones already reported by Merlin. The most useful errors enabled by the dune-diagnostics are signature mismatches errors, that Merlin is not able to provide. We might want to only report these for the active buffer ? This would also solve the issue of having duplicated type errors when the compiler prints them differently by Merlin. |
I don't want to see those either, because again, if I make it match the signature without saving, it will be red for me. In fact I'm much rather have a false positive that I discover when trying to compile than a false negative that is annoying me while working. But this is clearly a personal opinion and doesn't match other's experiences, so I would actually like the flag. In my opinion, having dune diagnostics for any type error is just a failure of Merlin and we should work on that. |
There was a problem hiding this comment.
Made a first round of review with a few remarks. I believe there is some cleaning to do in the Dune module.
I am not opposed to adding a flag to let users choose if they want to use the feature or not. It's sad that the LSP protocol doesn't provide a standardize way to specify such simple options.
|
|
||
| /** | ||
| * Enable/Disable Dune diagnostics | ||
| * @default false |
There was a problem hiding this comment.
The default is actually true
ocaml-lsp-server/src/config_data.ml
Outdated
| ; dune_diagnostics : DuneDiagnostics.t | ||
| [@key "duneDiagnostics"] | ||
| [@default DuneDiagnostics.{ enable = true }] | ||
| [@yojson_drop_default ( = )] |
There was a problem hiding this comment.
Do you know why previous options were added as Json.Nullable_option.t ? What is different with that new one ?
There was a problem hiding this comment.
I don't know, my bet is that it was to be able to distinguish between default and null, but it doesn't matter in any of them, as they all treat both options the same, I can change the other ones also if you want
@aspeddro may know better
There was a problem hiding this comment.
To help here I moved all of them to the simpler format.
There was a problem hiding this comment.
The simpler format is fine, but we'd appreciate it if it was done in a separate PR.
There was a problem hiding this comment.
I placed the simpler type in a different commit, to make it easier to review. If you want I can remove it, but I would be glad if we can avoid bikeshedding here.
There was a problem hiding this comment.
If my comments are "bikeshedding", then you will have a difficult time contributing to this repository. I like to keep PR's focused on one of: bug fixing, new features, refactoring. So yes, keep it focused for the reviewers and move the unrelated stuff to separate PR's.
ocaml-lsp-server/src/dune.mli
Outdated
| @@ -8,6 +8,9 @@ end | |||
|
|
|||
| type t | |||
|
|
|||
| (** enabled by default *) | |||
| val enable_diagnostics : bool ref | |||
There was a problem hiding this comment.
As far as I can see you never update this reference. Are the modifications in this module leftovers from a previous implementation ?
There was a problem hiding this comment.
Yup, nice catch, my bad I forgot to review the new implementation.
| Diagnostics.set_report_dune_diagnostics | ||
| (State.diagnostics state) | ||
| ~report_dune_diagnostics: | ||
| state.configuration.data.dune_diagnostics.enable |
There was a problem hiding this comment.
I'm not really fan of relying on a reference to update the configuration of the diagnostics, but I guess we have no choice ?
Also it looks like you are updating the config with the old value. The new value is in configuration.data.dune_diagnostics.enable, not state.configuration.data.dune_diagnostics.enable.
There was a problem hiding this comment.
Nice catch, yeah I had fixed this bug in the other branch but forgot to push it here.
There was a problem hiding this comment.
I would also like to not use mutation in general, but Diagnostics already had mutation, so I decided to go with that as the alternative would require quite a bit of work due to concurrency and already mutable state.
EduardoRFS
force-pushed
the
5.2-no-dune
branch
2 times, most recently
from
a93a46a
to
f0bb3cb
Compare
EduardoRFS
force-pushed
the
5.2-no-dune
branch
from
8b76ad7
to
468e4c3
Compare
|
Tested this patch with the VSCode OCaml extension and it's working as intended, both codelens and duneDiagnostics. |
Pull Request Test Coverage Report for Build 4079
💛 - Coveralls |
| | _ -> now []) | ||
| | TextDocumentCodeLens req -> | ||
| if state.configuration.data.codelens.enable then | ||
| later text_document_lens req |
There was a problem hiding this comment.
These changes seem unrelated. Separate PR for them please.
EduardoRFS
force-pushed
the
5.2-no-dune
branch
3 times, most recently
from
03d17b5
to
054b9e2
Compare
EduardoRFS
force-pushed
the
5.2-no-dune
branch
from
054b9e2
to
c6e6d43
Compare
|
Just tested the patch, it is working. |
|
Alright, this is good to go after some minor modifications. |
|
Thank you |
|
Thank you so much for this update! 🙇♂️ |
|
I think this will improve my experience as well. |
lint change from italics to code pill Completion for `in` keyword (ocaml#1217) feat(auto-completion): auto-complete `in` (in a hacky way) in .ml files Prepare release 1.17.0 (ocaml#1219) * chore: restore lost version number in changes (erased in c8c1096) * chore: bump version number in changes before release Add flag to disable dune diagnostics (ocaml#1221) * add config to control dune diagnostics lint update changelog
Context
I like to work with incomplete files a lot and playing around with them before actually saving, going through many intermediary steps.
But due to how OCaml LSP behaves as soon as you save a file with a single error coming from dune, that error will be there until you save it again, which is quite annoying.
So I've been using for almost 2 years a patched version of the OCaml LSP without dune diagnostics, this is hopefully that but as an opt-in option.
Patch
I tried to make the patch the least invasive, keeping all the features except the dune reporting inside of files, to do that I disable the entire diagnostics loop, this may be too much, but in the last years I never detected any issues.
How to use
{ "ocaml.sandbox": { "kind": "global" }, "ocaml.server.args": [ "--no-dune-diagnostics" ] }Related