search

haskell / lsp

Haskell library for the Microsoft Language Server Protocol
360 stars 89 forks source link

lsp-test: export runSessionWithConfig' to allow modifying the CreateProcess #449

Closed thomasjm closed 1 year ago

thomasjm commented 1 year ago

The default runSessionWithConfig launches an LSP server process, but doesn't provide any flexibility for modifying the CreateProcess to be used. This prevented me from doing things like tweaking the environment variables for the process.

This PR exports a new function runSessionWithConfig', where the first argument allows the caller to modify the CreateProcess.

thomasjm commented 1 year ago

My best alternative is just letting people pass the CreateProcess directly.

FWIW callers can already use a preexisting process if they call the lower-level runSessionWithHandles, which is similar but leaves the (somewhat tricky) handle setup to the caller.

As for the caller passing the CreateProcess, my thought was that that would be worse because then the caller has no way to inspect or change the extra changes that may be applied by the library (in Language.LSP.Test.Server). Currently those changes just involve setting up the streams but that could potentially change, and it would be frustrating for the library to be changing your CreateProcess in mysterious ways you can't adjust. So I went for maximum generality.

michaelpj commented 1 year ago

Currently those changes just involve setting up the streams but that could potentially change, and it would be frustrating for the library to be changing your CreateProcess in mysterious ways you can't adjust. So I went for maximum generality.

Okay, so I guess I'm wondering why you don't just call the withHandles version? Yes, it's a little bit of extra code, but not so much? I doubt we're really likely to do much setup except for spawning a process and setting up streams in future, so it's not like you're at that much risk of missing out on useful things that the library does for you.

thomasjm commented 1 year ago

Okay, so I guess I'm wondering why you don't just call the withHandles version?

Well, I just have a healthy fear of setting up these streams. If you do it wrong you can get weird blocking behavior or delays in message processing. I'm not even 100% sure this library does it optimally now, since there are a few places in haskell-language-server where the stderr is set to LineBuffering rather than NoBuffering.

As I use this library more I sort of anticipate tweaking Server.hs for this reason, and in any case I think getting it right is exactly what this library should take care of rather than burdening the user. Of course you're right that I could do it that way though.

michaelpj commented 1 year ago

Fair enough!

thomasjm commented 1 year ago

Great! I renamed it runSessionWithConfigCustomProcess and fixed up the Haddocks, should be ready to merge.