Better ocamlformatrpc integration #549

panglesd · 2021-11-15T15:12:30Z

This PR addresses issue #495.

In case of generated code via destruct or "inferred interface", ocaml-lsp now tries to format them using ocamlformat-rpc. If this fails, the non-formatted text is used. (When there are "merlin holes" no formatting is possible untill the merged Handle merlin typed holes ocaml-ppx/ocamlformat#1698 is released).
In case of user querying formatting the whole file, ocaml-lsp first tries ocamlformat-rpc and fallback to regular ocamlformat if it fails.

Signed-off-by: Paul-Elliot <[email protected]>

rgrinberg · 2021-11-17T03:17:02Z

The way this works is by sending formatting snippets, but the end result isn't necessarily formatted. I'm curious if you tested this out, does it seem to work well in practice?

rgrinberg · 2021-11-17T03:17:15Z

ocaml-lsp-server/src/code_actions/action_destruct.ml

+      let+ formattedText =
+        Ocamlformat_rpc.(format_type state.ocamlformat_rpc ~typ:newText)
+      in
+      match formattedText with


formatted_text

I'll update my variable names. Should newText also be renamed to new_text, or has this variable a reason to be formatted in camelCase?

camelCase naming is fine if used later for punning, I believe

rgrinberg · 2021-11-17T03:17:53Z

ocaml-lsp-server/src/code_actions/action_destruct.ml

-    | Ok res -> Some (code_action_of_case_analysis doc uri res)
+    | Ok (loc, newText) -> (
+      let+ formattedText =
+        Ocamlformat_rpc.(format_type state.ocamlformat_rpc ~typ:newText)


Why do you need the module open? I think Ocamlformat_rpc.format_type should be enough.

You are right, it is enough. I don't remember why I opened it.

rgrinberg · 2021-11-17T03:18:27Z

ocaml-lsp-server/src/code_actions/action_inferred_intf.ml

-    Some (code_action_of_intf doc intf params.range)
+  | Intf -> (
+    let* intf = Inference.infer_intf ~force_open_impl:true state doc in
+    let+ formattedIntf =


formatted_intf

panglesd · 2021-11-17T10:46:23Z

The way this works is by sending formatting snippets, but the end result isn't necessarily formatted. I'm curious if you tested this out, does it seem to work well in practice?

It works, I would say, better than without formatting, even though a subsequent call to ocamlformat for the whole file is needed to have the right formatting. However, I don't think automatically formatting the whole file is good for user experience, as it modifies the file at position far from the cursor without telling the user.

As a comparison, emacs does the same (formatting the generated code only), but is better just in the fact that it indent well the generated lines (but I think this is thanks to tuareg-mode and not merlin-related).

ulugbekna · 2021-11-18T13:25:05Z

Just to be sure, it seems that when using ocamlformat rpc the version put in .ocamlformat is not respected, eg see the video below where I have ocamlformat master pinned, and formatting works with .ocamlformat version=0.19.0 (but sometimes it doesn't -- I don't quite understand; it would be nice to make sure the rpc respects the .ocamlformat options, for example, as a test)

Screen.Recording.2021-11-18.at.14.22.33.mov

rgrinberg · 2021-11-22T01:45:45Z

Merged after a few tweaks.

ulugbekna · 2021-11-22T13:03:35Z

AFAIU, Ocamlformat_rpc.configure fn, which is called in Ocamlformat_rpc.(create : unit -> t), specifies formatting config tailored to suit "vscode popups", which isn't what we want when we format source files (we want to get the config from the .ocamlformat file). Shall we revert and let @panglesd continue working on the PR or have a follow up PR?

rgrinberg · 2021-11-22T13:43:09Z

panglsed can keep working on it in a separate PR. Even without the configuration, it's still an improvement over no formatting at all. Rudi.

…

On Nov 22, 2021, 7:03 AM -0600, Ulugbek Abdullaev ***@***.***>, wrote: AFAIU, Ocamlformat_rpc.configure fn, which is called in Ocamlformat_rpc.(create : unit -> t), specifies formatting config tailored to suit "vscode popups", which isn't what we want when we format source files (we want to get the config from the .ocamlformat file). Shall we revert and let @panglesd continue working on the PR or have a follow up PR? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

ulugbekna · 2021-11-22T13:52:35Z

I think you're missing that this PR formats source files as well, and the current master is pretty much unusable since any source file reformatted incorrectly on save (if they have formatting on save on). @panglesd do you have time to fix that?

panglesd · 2021-11-22T15:00:33Z

Sure, I'll try to fix this soon (I hope by tomorrow evening)

rgrinberg · 2021-11-22T15:57:35Z

We'll give you the week :)

panglesd · 2021-11-23T17:40:54Z

I am not anymore sure that we should format the entire file with ocamlformat_rpc.

As ocamlformat_rpc acts completely independently from the .ocamlformat file, we need to find and parse this file, to send to the ocamlformat_rpc server all the options of .ocamlformat.
The problem is that ocamlformat_rpc does not have a counterpart to the "version" value in the .ocamlformat file: To fail on wrong versions, we would also need to parse the file to find the required version, call ocamlformat-rpc --version to know the installed version, compare the two and fail if they differ.

I start to think that there are more disadvantages in using ocamlformat_rpc instead of calling ocamlformat directly, as we need to reinvent the wheel for two reasons: sending .ocamlformat options, and compare .ocamlformat version with installed version. Thats' a lot!

What do you think? Should I revert back to ocamlformat for formatting whole file? Or should continue the ocamlformat_rpc way? (I have something working except for the version comparison)

rgrinberg · 2021-11-23T17:45:37Z

IMO this is a bug in ocamlformat_rpc's server. I don't see how it makes sense for their rpc server to just ignore the .ocamlformat file. It's clearly an implementation detail when using the binary, so why is using the protocol any different? Rudi.

…

On Nov 23, 2021, 11:41 AM -0600, panglesd ***@***.***>, wrote: I am not anymore sure that we should format the entire file with ocamlformat_rpc. As ocamlformat_rpc acts completely independently from the .ocamlformat file, we need to find and parse this file, to send to the ocamlformat_rpc server all the options of .ocamlformat. The problem is that ocamlformat_rpc does not have a counterpart to the "version" value in the .ocamlformat file: To fail on wrong versions, we would also need to parse the file to find the required version, call ocamlformat-rpc --version to know the installed version, compare the two and fail if they differ. I start to think that there are more disadvantages in using ocamlformat_rpc instead of calling ocamlformat directly, as we need to reinvent the wheel for two reasons: sending .ocamlformat options, and compare .ocamlformat version with installed version. Thats' a lot! What do you think? Should I revert back to ocamlformat for formatting whole file? Or should continue the ocamlformat_rpc way? (I have something working except for the version comparison) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

ulugbekna · 2021-11-23T18:24:44Z

That indeed seems limitation on the ocamlformat-rpc side, which should be fixed.

I don't know how much we would like to handle .ocamlformat files ourselves, but

parsing ocamlformat files is easy
we anyway would like to add support for auto-completion in ocamlformat files, which should involve parsing those files anyway
we could add various features based on handling ocamlformat files, e.g., on ocamlformat file change propose to reformat all files (preferably with debouncing however)

rgrinberg · 2021-11-23T18:33:45Z

I don't mind adding these features but I want all the heavy lifting to be on the ocamlformat side through rpc. It's their format and its interpretationshould be a black box to us as much as possible.

on ocamlformat file change propose to reformat all files (preferably with debouncing however)

That's something that should be done through dune.

panglesd · 2021-11-23T22:27:48Z

parsing ocamlformat files is easy
Sure, but I agree with Rudi that its their format so parsing should be done on their side.

I will make a PR reverting file formatting to using ocamlformat, and file an issue to the ocamlformat repo about using .ocamlformat for the rpc server.

rgrinberg · 2021-11-23T22:37:48Z

Instead of reverting you can also add a flag to disable it. Might save the effort of having to reintroduce the patch after the issue is fixed upstream.

panglesd added 3 commits November 15, 2021 15:16

format code generated by merlin destruct (if it can)

3360a2a

Signed-off-by: Paul-Elliot <[email protected]>

format code for generated interface (if it can)

2776ed8

Signed-off-by: Paul-Elliot <[email protected]>

Format user queries with ocamlformat_rpc, fallback to ocamlformat

28fc89e

Signed-off-by: Paul-Elliot <[email protected]>

ulugbekna self-requested a review November 15, 2021 15:19

panglesd mentioned this pull request Nov 15, 2021

Better ocamlformat-rpc intergration #495

Closed

4 tasks

rgrinberg reviewed Nov 17, 2021

View reviewed changes

snake_case

46a7410

rgrinberg closed this Nov 22, 2021

panglesd mentioned this pull request Nov 24, 2021

Separate ocamlformat_rpc servers for popup and for file formatting #554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better ocamlformatrpc integration #549

Better ocamlformatrpc integration #549

panglesd commented Nov 15, 2021

rgrinberg commented Nov 17, 2021

rgrinberg Nov 17, 2021

panglesd Nov 17, 2021

ulugbekna Nov 18, 2021 •

edited

Loading

rgrinberg Nov 17, 2021

panglesd Nov 17, 2021

rgrinberg Nov 17, 2021

panglesd commented Nov 17, 2021

ulugbekna commented Nov 18, 2021

rgrinberg commented Nov 22, 2021

ulugbekna commented Nov 22, 2021

rgrinberg commented Nov 22, 2021 via email

ulugbekna commented Nov 22, 2021

panglesd commented Nov 22, 2021

rgrinberg commented Nov 22, 2021

panglesd commented Nov 23, 2021

rgrinberg commented Nov 23, 2021 via email

ulugbekna commented Nov 23, 2021 •

edited

Loading

rgrinberg commented Nov 23, 2021 •

edited

Loading

panglesd commented Nov 23, 2021

rgrinberg commented Nov 23, 2021

Better ocamlformatrpc integration #549

Better ocamlformatrpc integration #549

Conversation

panglesd commented Nov 15, 2021

rgrinberg commented Nov 17, 2021

rgrinberg Nov 17, 2021

Choose a reason for hiding this comment

panglesd Nov 17, 2021

Choose a reason for hiding this comment

ulugbekna Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

rgrinberg Nov 17, 2021

Choose a reason for hiding this comment

panglesd Nov 17, 2021

Choose a reason for hiding this comment

rgrinberg Nov 17, 2021

Choose a reason for hiding this comment

panglesd commented Nov 17, 2021

ulugbekna commented Nov 18, 2021

rgrinberg commented Nov 22, 2021

ulugbekna commented Nov 22, 2021

rgrinberg commented Nov 22, 2021 via email

ulugbekna commented Nov 22, 2021

panglesd commented Nov 22, 2021

rgrinberg commented Nov 22, 2021

panglesd commented Nov 23, 2021

rgrinberg commented Nov 23, 2021 via email

ulugbekna commented Nov 23, 2021 • edited Loading

rgrinberg commented Nov 23, 2021 • edited Loading

panglesd commented Nov 23, 2021

rgrinberg commented Nov 23, 2021

ulugbekna Nov 18, 2021 •

edited

Loading

ulugbekna commented Nov 23, 2021 •

edited

Loading

rgrinberg commented Nov 23, 2021 •

edited

Loading