cuelsp icon indicating copy to clipboard operation
cuelsp copied to clipboard

feat: add fmt capabilities

Open b4nst opened this issue 3 years ago • 10 comments

An attempt at including formatting to the LSP. I'm quite new to LSP implementation, not sure if there's a standard for passing options to the formatter. I'll dig a bit in the doc later see if I find something useful.

Resolves #44

b4nst avatar Jul 31 '22 11:07 b4nst

Went a bit too fast on this one. I need to get the file from the workspace and not a dumb os.ReadFile, otherwise it doesn't take current changes into account 🤦 .

b4nst avatar Jul 31 '22 21:07 b4nst

Hey, thanks for your time and contribution :rocket:

Went a bit too fast on this one. I need to get the file from the workspace and not a dumb os.ReadFile, otherwise it doesn't take current changes into account 🤦 .

Actually I think it's ok to keep the same logic as cue fmt and format the full file. That's maybe a simpler implementation for now?

TomChv avatar Aug 01 '22 05:08 TomChv

I don't see any issue formatting the whole file. However when doing some tests, it seems the os.ReadFile is happening before the file is actually saved. I guess that's the normal flow of an on save hook: hit save => triggers hooks => actually save with the modified file. Meaning I ended up overriding my changes with the last state saved in file. It's also maybe an issue with the lsp client (I'm using Helix editor embedded client) that should send the current buffer instead of the local path. Not sure of the right client implementation for this feature.

b4nst avatar Aug 03 '22 01:08 b4nst

matting the whole file. However when doing some tests, it seems the os.ReadFile is happening before the file is actually saved. I guess that's the normal flow of an on save hook: hit save => triggers hooks => actually save with the modified file. Meaning I ended up overriding my cha

I'll take some time to explore today 😇

grouville avatar Aug 03 '22 11:08 grouville

I wonder if we should start implementing documentDidChange and reload the plan with changes. And then use the plan itself to somehow get the file content, or the ast.Node to feed it to format.Source or format.Node. I'm just a bit concerned by the performances of a plan reload with every changes.

b4nst avatar Aug 04 '22 00:08 b4nst

I ran some quick and dirty stuff here, using a simple sync.Map as a storage. And it does work.

So I guess updating the Plan and getting back the file directly from it should solve our issue. I'll explore a way to do so

b4nst avatar Aug 04 '22 01:08 b4nst

@grouville I updated my code to take the file directly from the plan and add an override system to the plan. It seems to work well but it feels kinda hacky tbh. I'm wonderning if we wouldn't be better using the Overlay capabilities of load.Config

b4nst avatar Aug 04 '22 18:08 b4nst

I'll take some time today to check your solution too

TomChv avatar Aug 04 '22 20:08 TomChv

@b4nst. I've been checking your solution, and it seems that it continues overwriting the user's input. It's more subtile, but it can still annoy the users:

https://user-images.githubusercontent.com/31691250/183421174-41071ad2-b760-4c31-8116-af4633cd8653.mov

Do you have an idea on where this happens ?

grouville avatar Aug 08 '22 12:08 grouville

Huum, maybe because the file didn't build correctly it was not in the list of files of the main Plan. I honestly have to give a better try to this one. It requires more work than this sort of hack

b4nst avatar Aug 09 '22 21:08 b4nst