Skip to content
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

Implement textDocument/formatting #89

Merged
merged 17 commits into from
Feb 18, 2020

Conversation

cannorin
Copy link
Contributor

This PR implements textDocument/formatting with a wrapper module to ocamlformat.

It finds ocamlformat from the current PATH and runs it at the current directory ocamllsp is running. Note that I didn't add any facility to find the nearest .ocamlformat.

textDocument/{rangeFormatting, onTypeFormatting} is not yet implemented. Once ocaml-ppx/ocamlformat#1188 comes into reality, we can easily extend this implementation to support them.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's get rid of all the things used to configure ocamlformat. Ocamlformat should read its own configuration by reading the .ocamlformat file (or otherwise choose a default) config.

@cannorin
Copy link
Contributor Author

OK, I'll remove those config stuffs.

@cannorin cannorin changed the title Implement textDocument_formatting Implement textDocument/formatting Feb 12, 2020
@cannorin
Copy link
Contributor Author

We will have to set --lines option to support textDocument/{rangeFormatting, onTypeFormatting}, so I'll leave minimal support of passing options instead of completely removing it.

@rgrinberg
Copy link
Member

Looking good. In this PR, we still need to add:

  • Tests (we may install ocamlformat for this)
  • Change the capabilities of the server to signal to the client that it can format documents.

@cannorin
Copy link
Contributor Author

Ok, but where the convention is documented?

@rgrinberg
Copy link
Member

Ok, but where the convention is documented?

Nowhere 😭 Once I switch to generated code, I will document the convention more thoroughly. For now, the simple rule is: any identifier from the LSP spec should maintain its casing. Otherwise, we should stick to the OCaml convention of [Ff]oo_bar

@cannorin
Copy link
Contributor Author

rebased.

@cannorin
Copy link
Contributor Author

@rgrinberg I revert efe2672. Stdune's read_all uses in_channel_length which is only meaningful for channels connected to regular files. In our case the channel is connected to the stdout of ocamlformat and it raises Sys_error("Illegal seek").

@cannorin
Copy link
Contributor Author

I guess we should add ocamlformat to the CI environment and I'm not sure how.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
cannorin and others added 3 commits February 18, 2020 15:30
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Member

I pushed a commit that handles errors a little better notifies the user if the ocamlformat binary is missing.

@rgrinberg rgrinberg merged commit b1168b8 into ocaml:master Feb 18, 2020
@cannorin cannorin deleted the textDocument_formatting branch February 18, 2020 17:08
@smorimoto smorimoto mentioned this pull request Feb 26, 2020
@cannorin cannorin mentioned this pull request Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants