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

Add docs on how to choose a formatter #432

Merged
merged 4 commits into from Oct 2, 2020

Conversation

googleson78
Copy link
Contributor

No description provided.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Hi!
I have a few questions, I would not know how this documentation is helpful, or how I would actually write it, could you clarify a bit?

README.md Outdated
Comment on lines 274 to 282
{
...
"haskell": {
...
"formattingProvider": "fourmolu"
...
}
...
}
Copy link
Collaborator

@fendor fendor Sep 25, 2020

Choose a reason for hiding this comment

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

What is this lsp configuration? I have never seen that one. What should be the name of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming it is a .json file that you point your lsp client to (e.g. in LanguageClient-neovim that seems to be g:LanguageClient_settingsPath)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is specific to LanguageClient-neovim? At least I have never seen such a configuration file for HLS itself. Others will have to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed it is generic, because I couldn't find another way of choosing a formatter. How do you do so in your editor+client?

Copy link
Collaborator

Choose a reason for hiding this comment

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

VSCode / VSCodium has a settings Tab where you can choose the formatter out of a dropbox menu

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok makes sense, so this configuration's location is still editor dependent. We should make this clear and maybe refer to the documentation of the respective lsp-client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed some "clarification", is it ok?

I'm also not sure how good an idea being this specific as I was is, because while it's helpful for someone who has never used their respective LSP client before, it might get outdated and is in a way "redundant" information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is quite client-specific. For example, in lsp-haskell (the emacs mode), you configure these things with emacs defcustom variables. I think it's reasonable for the client to explain how configuration is done, since we can't specify it in a general way on the server side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But there should still be some mention that you can put such a field into your client's configuration, whatever that may be. If it didn't exist in the changelog you would have to read through source code to learn about this.

Besides, there is already precedent for descriptions on how to configure different clients for use with HLS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I'm the only one who found it took effort to find this out either:
https://twitter.com/seagreen__/status/1309967145221214209?s=20

@@ -253,6 +254,34 @@ If your desired ghc has been found, you use it to install haskell-language-serve
./cabal-hls-install data
```

## HLS LSP Configuration

haskell-language-server supports some forms of configuration.
Copy link
Collaborator

@fendor fendor Sep 25, 2020

Choose a reason for hiding this comment

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

For what editor? For all? Is that a HLS configuration I was not aware of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jneira
Copy link
Member

jneira commented Sep 26, 2020

@googleson78 i knew we had something related with the configuration in the repo but i did not remember it was a pr so i opened a issue that will be closed by this.
Thanks for improve the docs: i agree that is needed to have here a section about in the server itself, indepedently of docs in each editor/lsp client. As reference for all of them and to make clear how an user can change the behaviour of the server.

@jneira
Copy link
Member

jneira commented Sep 29, 2020

@fendor @michaelpj dont want to bother but do you think there are still some concrete change to do? maybe something like "the way to update the config varies per editor but... for example in vscode is done this way ..."?

@michaelpj
Copy link
Collaborator

I guess what I worry about is: say I change the name of a variable in lsp-haskell. Very good, I update the documentation there, but I forget that I also have to go and modify the haskell-language-server documentation. Let's not even think about the fact that people might be using different versions of lsp-haskell, so we can't give universally applicable configuration information.

So it seems fragile to me to give specific information about other projects when we have no guarantee about what version of those projects users will be using.

@googleson78
Copy link
Contributor Author

googleson78 commented Sep 29, 2020

@michaelpj
In general I agree with you - this is why I originally wanted to only mention "this is what your final json lsp config should contain", because as far as I understand the name [haskell.]formattingProvider is determined by hls, so it shouldn't be an issue. (and because the "final json config" seems to be a universal thing among LSP clients)

I only added the more "detailed" descriptions because my original one seemed to cause confusion, even among experienced hls users.

They're also in the spirit of the editor setup examples that are present in the current README (they also have the same issues you described - upstream could change etcetc) of leaning towards user-friendliness at the cost of maintenance effort.

@michaelpj
Copy link
Collaborator

and because the "final json config" seems to be a universal thing among LSP clients

It's not, though. Certainly not for emacs clients. Selfishly, I expect this would lead to people opening lsp-haskell issues saying "where do I put the JSON config?".

I don't know. Maybe I'm just generalizing from myself too much, but I'd much rather see "For editor X, use Y, and see Y's documentation for setting it up (LINK)". I can click on links!

@googleson78
Copy link
Contributor Author

By "final", I mean that "in the end" (at the lowest level) in communication with the LSP server there is some json (if it's actually json??) value being sent, regardless of the surface format presented by the editor/lsp client.

I'm open to other suggestions to describe this in a client agnostic way ("you need to make your client send the haskell.formattingProvider value"), after which we can remove the client-specific descriptions and leave only links to their specific ways for configuration

@michaelpj
Copy link
Collaborator

By "final", I mean that "in the end" (at the lowest level) in communication with the LSP server there is some json (if it's actually json??) value being sent, regardless of the surface format presented by the editor/lsp client.

Yeah, but it's not the JSON that you see in your VSCode settings.

I'm open to other suggestions to describe this in a client agnostic way ("you need to make your client send the haskell.formattingProvider value")

The client-agnostic way is to describe the LSP protocol messages the client needs to send, which is not what the user wants to see ;)

@googleson78
Copy link
Contributor Author

googleson78 commented Sep 29, 2020

Well.. it's exactly what I wanted to see, so I had some clue what I needed to look for in my LSP client (e.g. if it's not documented/present I can open an issue and ask how to do that there).

In any case, it seems that there needs to be some choice made wrt user friendliness vs maintenance burden here, and I'm not the one to make that choice.

@jneira
Copy link
Member

jneira commented Sep 29, 2020

Well, i think we agree in is good to include:

  • the server can be configured in the client in some way
  • one of the configuration settings is the formatter provider, and the actual list of possible values

So removing the reference to the concrete json would make it client agnostic enough?

@googleson78
Copy link
Contributor Author

I removed the json, but still retained a mention of what the key name is in raw json. If I were to remove that also and delete my knowledge of how to set it, I would still have no idea how to choose a formatting provider by looking at the README

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

LGTM in its actual phrasing.
//cc @fendor

@jneira
Copy link
Member

jneira commented Oct 2, 2020

@michaelpj maybe we could add a reference to the emacs way to change settings in a followup, if you think it would be helpful for emacs users

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jneira jneira merged commit d4d50a6 into haskell:master Oct 2, 2020
@jneira jneira mentioned this pull request Mar 30, 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.

Document how hls can be configured
4 participants