Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 414
Add formatting plugin for cabal files which uses cabal-fmt#2047
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
VeryMilkyJoe commented Jul 28, 2021 • edited by fendor
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by fendor
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jneira commented Jul 28, 2021
thanks you much for this nice plugin |
Uh oh!
There was an error while loading. Please reload this page.
pepeiborra commented Jul 29, 2021
I think that you need to run the test suite explicitly in the test GitHub action: haskell-language-server/.github/workflows/test.yml Lines 200 to 206 in c2bd211
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e5ed4d8 to 8e06b38CompareVeryMilkyJoe commented Aug 4, 2021
I had to remove stack support as cabal-fmt requires Cabal 3.4 but it seems impossible to make stack use Cabal 3.4 due to stack's design philosophy. |
Ailrun left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate the stack issue more? I believe stack works well with extra-deps?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
fendor commented Aug 4, 2021
Great work! I love the idea to have cabal formatting available in HLS! |
VeryMilkyJoe commented Aug 5, 2021
You might be right, I'm going to look into it some more. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
VeryMilkyJoe commented Aug 6, 2021
You now get a bunch of error messages when opening a cabal file since the client requests certain actions for which we have no plugins for cabal files. |
Uh oh!
There was an error while loading. Please reload this page.
jneira commented Sep 1, 2021
@VeryMilkyJoe hi! i am afraid that the pr has to be rebased on master |
jneira commented Sep 16, 2021
@VeryMilkyJoe i did a rebase, i hope it is correct, do you have plans to continue working on this? thanks! |
1c38e53 to 721047eComparee25fcf4 to 2ae9f02Compare9dbd1bf to 2790cfaComparecde554f to 631a006Compare| ++ ["failed with standard error:"<+> pretty stdErrorOut |not (null stdErrorOut)] | ||
| LogInvalidInvocationInfo->"Invocation of cabal-fmt with range was called but is not supported." | ||
| descriptor::Recorder (WithPriorityLog) ->PluginId->PluginDescriptorIdeState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide a configuration option to let users specify the path of cabal-fmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that's overkill, probably cabal-fmt on path is going to be the thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good addition!
| provider::Recorder (WithPriorityLog) ->FormattingHandlerIdeState | ||
| provider recorder _ (FormatRange _) _ _ _ =do | ||
| logWith recorder InfoLogInvalidInvocationInfo | ||
| pure$Left (ResponseErrorInvalidRequest"You cannot format a text-range using cabal-fmt."Nothing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we not have a better way to have a formatting provider that only supports whole buffer formatting? I think some of our others have this limitation too 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all formatter plugins (stylish-haskell, ormolu, fourmolu, brittany, floskell), everyone seems to handle ranges :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we could do something clever with dynamic registration to say we only support it in certain circumstances... but that seems hard. This seems okay I guess. Worth a comment, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of comment are you thinking here?
| let fmtDiff = makeDiffTextEdit contents (T.pack out) | ||
| pure $ Right fmtDiff | ||
| Nothing -> do | ||
| pure $ Left (ResponseError InvalidRequest "No installation of cabal-fmt could be found. Please install it into your global environment." Nothing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drsooch did we do this?
Uh oh!
There was an error while loading. Please reload this page.
michaelpj commented Nov 5, 2022
I'd be happy to merge this as is and do improvements in follow ups, WDYT @fendor ? |
fendor commented Nov 5, 2022
2-3 improvements are still missing, @VeryMilkyJoe is very responsive and will incorporate the requested changes until tomorrow. |
michaelpj commented Nov 5, 2022
There's not any hurry, I just thought you both might like to have it in :) |
fendor left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's get this going :)
For CI, we want to run the tests with a specific cabal-fmt version, installed automatically by cabal. However, locally, we might want to test with a locally installed cabal-fmt version. This flag allows developers to either let cabal install the build-tool-depends or install a fitting version locally.
This allows formatting of cabal files with hls.
Currently waiting for merges on cabal-fmt and stylish haskell.We gave up on this, also it makes maintenance harder as cabal-fmt supports only one Cabal version at a time.Closes#183 and part of #2964.
Builds on top of #2945