Support textDocument/rangeFormatting#1591
Conversation
|
Why not ocamlformat the whole document but don't bring in changes to the before and after areas of the document. This approach may allow selecting arbitrary regions for range formatting and is more flexible ? |
I see two problems with this:
|
|
The problem with your suggested approach is also that you need to be conscious of whether the snippet you select constitutes a valid ocaml file if it were "copy + pasted" into a separate ocaml file. Sometimes you only want to range format say the inside of a for loop -- that snippet may not be valid top level code. Typically when you are editing a file in an editor, the document as a whole enters invalid syntax occasionally but when you're asking for formatting it is not too big a deal to need provide a syntactically correct ocaml file.
In your logic you could add a ocaml comment |
|
At least, I think that having the ability to format one range is very useful (and easier to maintain at the editor level, mostly because it fits with the LSP specification). |
xvw
left a comment
xvw
left a comment
There was a problem hiding this comment.
I have some nitpicking comments (feel free to resolve them if you do not agree, I do not have strong opinion) but I think 3 things are missing:
- Some tests to illustrate the behaviour of the command (in e2e-new)
- Extracting the behaviour of the contextual formatting and testing it (that can be useful for formatting in context of other command like destruct and construct for example)
- A change entry :)
Thanks a lot for the solid contribution!
ocaml-lsp-server/src/ocamlformat.ml
Outdated
| let contents = Document.source doc |> Msource.text in | ||
| let start, stop = Text_document.absolute_range (Document.tdoc doc) range in | ||
| (* basic idea: | ||
| - slice out the range to be formatted, send to ocamlformat | ||
| - whitespace pad the start of lines in the reply based on the selection start | ||
| - stitch everything back together. *) | ||
| let prefix, to_format, suffix = | ||
| ( String.sub contents ~pos:0 ~len:start | ||
| , String.sub contents ~pos:start ~len:(stop - start) | ||
| , String.sub contents ~pos:stop ~len:(String.length contents - stop) ) | ||
| in | ||
| let args = args formatter in | ||
| let args = | ||
| (* if we're formatting the start of a [let ... in] construct, | ||
| don't emit [;;] before the [in]! *) | ||
| let next = | ||
| String.trim suffix ~drop:(function | ||
| | ' ' | '\n' | '\r' | '\t' -> true | ||
| | _ -> false) | ||
| in | ||
| if String.is_prefix ~prefix:"in" next || String.is_prefix ~prefix:";;" next | ||
| then "--let-binding-spacing=compact" :: args | ||
| else args | ||
| in | ||
| let column_offset = range.start.character in | ||
| let pad s = | ||
| let padding = Bytes.make column_offset ' ' |> Bytes.to_string in | ||
| String.concat ~sep:"\n" (List.map (String.split_lines s) ~f:(( ^ ) padding)) | ||
| |> String.trim ~drop:(( = ) ' ') | ||
| in | ||
| let open Fiber.O in | ||
| let* margin = compute_modified_margin binary cancel column_offset formatter in | ||
| let args = margin :: args in | ||
| let+ r = exec cancel binary args to_format in | ||
| Result.map r ~f:(fun { stdout = formatted; _ } -> | ||
| let to_ = prefix ^ pad formatted ^ suffix in | ||
| Diff.edit ~from:contents ~to_) |
There was a problem hiding this comment.
I think this function should be extracted/exported (and unit tested) to make it easier to reuse and maintain. What do you think?
There was a problem hiding this comment.
I'm not entirely sure what function you're referring to here -- the padding function?
There was a problem hiding this comment.
The whole manipulation on formatted fragment
There was a problem hiding this comment.
Let me know if the new format_snippet function is what you had in mind. If so, I can start adding separate unit tests.
|
Since you add a new capability, you need to upgrade the test-suite ( |
|
@xvw I would like some additional guidance on what you meant by extracting the contextual formatting capability (mainly, where you see the cut point in the current code/what the ideal signature of the extracted function would be, in your eyes) Other than that, I believe I have addressed your comments. For the e2e-new testing, I also ported the old |
|
Thanks a lot! |
3 participants
This PR implements the
textDocument/rangeFormattingrequest for ocaml-lsp-server.The basic idea is this:
before, the piece we want to format, andafter.beforeandafterand generate theTextEditsfrom this result.The
*in step two is because, if we want nice results, we need to modify our call to ocamlformat in the following ways:Because our selection may be at some deep indentation level in the surrounding code, special care is needed to preserve this. The approach I've taken here is to
c.f. Feature request: Ability to request starter indentation level ocaml-ppx/ocamlformat#2773
If the snippet you're highlighting is a
let ... inwhere you've left of thein, some ocamlformat configs will desire to add a;;at the end, which is annoying and breaks the surrounding code, so I special case this.This PR closes ocamllabs/vscode-ocaml-platform#1891