-
-
Notifications
You must be signed in to change notification settings - Fork 367
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 formatting plugin for cabal files which uses cabal-fmt #2047
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
thanks you much for this nice plugin |
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
|
e5ed4d8
to
8e06b38
Compare
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. |
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
?
Great work! I love the idea to have cabal formatting available in HLS! |
You might be right, I'm going to look into it some more. |
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.
|
@VeryMilkyJoe hi! i am afraid that the pr has to be rebased on master |
@VeryMilkyJoe i did a rebase, i hope it is correct, do you have plans to continue working on this? thanks! |
1c38e53
to
721047e
Compare
e25fcf4
to
2ae9f02
Compare
9dbd1bf
to
2790cfa
Compare
cde554f
to
631a006
Compare
++ ["failed with standard error:" <+> pretty stdErrorOut | not (null stdErrorOut)] | ||
LogInvalidInvocationInfo -> "Invocation of cabal-fmt with range was called but is not supported." | ||
|
||
descriptor :: Recorder (WithPriority Log) -> PluginId -> PluginDescriptor IdeState |
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 (WithPriority Log) -> FormattingHandler IdeState | ||
provider recorder _ (FormatRange _) _ _ _ = do | ||
logWith recorder Info LogInvalidInvocationInfo | ||
pure $ Left (ResponseError InvalidRequest "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?
I'd be happy to merge this as is and do improvements in follow ups, WDYT @fendor ? |
2-3 improvements are still missing, @VeryMilkyJoe is very responsive and will incorporate the requested changes until tomorrow. |
There's not any hurry, I just thought you both might like to have it in :) |
631a006
to
d6b6d89
Compare
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