Skip to content
This repository has been archived by the owner on Jan 16, 2024. It is now read-only.

feat: add fmt capabilities #84

Closed
wants to merge 3 commits into from
Closed

Conversation

b4nst
Copy link

@b4nst b4nst commented Jul 31, 2022

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
Copy link
Author

b4nst commented Jul 31, 2022

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 🤦 .

@TomChv TomChv added the area/server All about server label Aug 1, 2022
@TomChv
Copy link
Member

TomChv commented Aug 1, 2022

Hey, thanks for your time and contribution 🚀

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?

@b4nst
Copy link
Author

b4nst commented Aug 3, 2022

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.

Copy link
Member

@grouville grouville left a comment

Choose a reason for hiding this comment

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

I've tested locally, and it seems that it overrides the local changes, prior saving. A solution would be to maybe move that function to the onsave handler, or find a way to update before that (doing a reload of the plan)

@grouville
Copy link
Member

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 😇

@b4nst
Copy link
Author

b4nst commented Aug 4, 2022

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
Copy link
Author

b4nst commented Aug 4, 2022

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 b4nst force-pushed the add/fmt-capability branch from 8ea6580 to 75e7edb Compare August 4, 2022 18:22
@b4nst
Copy link
Author

b4nst commented Aug 4, 2022

@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

@TomChv
Copy link
Member

TomChv commented Aug 4, 2022

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

@grouville
Copy link
Member

@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:

Enregistrement.de.l.ecran.2022-08-08.a.14.44.31.mov

Do you have an idea on where this happens ?

@b4nst
Copy link
Author

b4nst commented Aug 9, 2022

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

@gerhard gerhard closed this Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/server All about server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File formatting
4 participants