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

Allow setting line width for refmt #172

Closed
wants to merge 2 commits into from

Conversation

rusty-key
Copy link
Contributor

@rusty-key rusty-key commented May 8, 2020

There is currently no way of setting up line width for refmt other than passing flag to the command.

While we at this point, I am suggesting to introduce --refmt-width argument to ocamllsp and pass it to refmt. On editor extension side a setting for refmt can be introduced and passed to ocamllsp instance: ocamllabs/vscode-ocaml-platform#184

Related issue in reason repo: reasonml/reason#2567

@giltho
Copy link
Collaborator

giltho commented May 9, 2020

Hey !
Following up on and summarizing what has been said in ocamllabs/vscode-ocaml-platform#184:
-> passing the option to the ocamllsp command line arguments is not the right way to do things

-> The best way is to pass the argument to the formatting options because it is what is intended to. However, in our case it might be a tiny bitsy more difficult to implement given that the FormattingOption type is generated and custom fields are not handled right now. The type generator could be extended with a mechanism that adds a custom_ field of type

[> `String of string, `Bool of boolean, `Int of int, `Float of float]

for this [key: string]: boolean | number | string
Once that is done the implementation is trivial both here and in the vscode extension.

-> The 2nd best way is to use workspace configuration. But then you would need to implement the whole configuration system which is probably even more trouble.

So I have not said anything new from what has already been said in the discussion on your vscode-ocaml-platform PR apart from implementation details. Feel free to ping if you need any help.

@rusty-key
Copy link
Contributor Author

Hey @giltho! Thanks for the summary. The part I am struggling with is actually on the client side: I cannot understand when and how to pass DocumentFormattingParams from the client. Looking at spec I thought that I need to register it via ClientCapabilities during initialization, but it looks like that's not what you are talking about. Can you direct me in the right direction?

@giltho
Copy link
Collaborator

giltho commented May 9, 2020

Ah damn... Took me a while to find it: https://github.com/microsoft/vscode-languageserver-node/blob/363e9228a87d3df51427474ecc596740c3fecfd5/client/src/codeConverter.ts#L616

The params sent by the vscode client are hardcoded... Language clients like Typescript's for VSCode are custom-written, and are not using the high-level LanguageClient class, so that's why they can use additional parameters...

So the first possiblity would be to fix the vscode-language client, but that's very much annoying, because we'd have to wait on the next vscode update....

I guess you should go with the workspace configuration option

@rgrinberg
Copy link
Member

@giltho is right. Let us know when this PR is updated and is ready for review again.

@rusty-key
Copy link
Contributor Author

@rgrinberg, I'll have some time over this weekend to refactor the PR

@rgrinberg
Copy link
Member

Please re-open when that weekend comes. Note that master already has the infrastructure for maintaining a Configuration.t value. So implementing this feature should be quite easy.

@rgrinberg rgrinberg closed this Apr 20, 2021
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.

3 participants