Skip to content
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

Merged
merged 4 commits into from
Nov 11, 2022

Conversation

VeryMilkyJoe
Copy link
Collaborator

@VeryMilkyJoe VeryMilkyJoe commented Jul 28, 2021

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

@Ailrun

This comment has been minimized.

@VeryMilkyJoe

This comment has been minimized.

@jneira
Copy link
Member

jneira commented Jul 28, 2021

thanks you much for this nice plugin
I think you will have to add the missing deps in the different stack.yaml's to make the stack builds work

@pepeiborra
Copy link
Collaborator

I think that you need to run the test suite explicitly in the test GitHub action:

- if: ${{ needs.pre_job.outputs.should_skip != 'true' && matrix.test && matrix.ghc != '9.0.1' }}
name: Test hls-refine-imports-plugin test suite
run: cabal test hls-refine-imports-plugin --test-options="-j1 --rerun-update" || cabal test hls-refine-imports-plugin --test-options="-j1 --rerun" || LSP_TEST_LOG_COLOR=0 LSP_TEST_LOG_MESSAGES=true LSP_TEST_LOG_STDERR=true cabal test hls-refine-imports-plugin --test-options="-j1 --rerun"
- if: ${{ needs.pre_job.outputs.should_skip != 'true' && matrix.test}}
name: Test hls-call-hierarchy-plugin test suite
run: cabal test hls-call-hierarchy-plugin --test-options="-j1 --rerun-update" || cabal test hls-call-hierarchy-plugin --test-options="-j1 --rerun" || LSP_TEST_LOG_COLOR=0 LSP_TEST_LOG_MESSAGES=true LSP_TEST_LOG_STDERR=true cabal test hls-call-hierarchy-plugin --test-options="-j1 --rerun"

@VeryMilkyJoe VeryMilkyJoe force-pushed the cabal-fmt branch 3 times, most recently from e5ed4d8 to 8e06b38 Compare August 3, 2021 16:48
@VeryMilkyJoe
Copy link
Collaborator Author

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.

Copy link
Member

@Ailrun Ailrun left a 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?

hls-plugin-api/src/Ide/Plugin/Config.hs Show resolved Hide resolved
hls-test-utils/src/Test/Hls.hs Show resolved Hide resolved
@fendor
Copy link
Collaborator

fendor commented Aug 4, 2021

Great work! I love the idea to have cabal formatting available in HLS!

@VeryMilkyJoe
Copy link
Collaborator Author

Could you elaborate the stack issue more? I believe stack works well with extra-deps?

You might be right, I'm going to look into it some more.

@VeryMilkyJoe
Copy link
Collaborator Author

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.

[Error - 12:07:11] Request textDocument/codeLens failed.
  Message: No plugin enabled for STextDocumentCodeLens, available: []
  Code: -32600 
[Error - 12:07:51] Request textDocument/codeLens failed.
  Message: No plugin enabled for STextDocumentCodeLens, available: []
  Code: -32600 
[Error - 12:09:07] Request textDocument/codeLens failed.
  Message: No plugin enabled for STextDocumentCodeLens, available: []
  Code: -32600 
[Error - 12:09:08] Request textDocument/codeAction failed.
  Message: No plugin enabled for STextDocumentCodeAction, available: []
  Code: -32600 
[Error - 12:09:08] Request textDocument/hover failed.
  Message: No plugin enabled for STextDocumentHover, available: []

ghcide/src/Development/IDE/LSP/HoverDefinition.hs Outdated Show resolved Hide resolved
@jneira
Copy link
Member

jneira commented Sep 1, 2021

@VeryMilkyJoe hi! i am afraid that the pr has to be rebased on master

@jneira
Copy link
Member

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!

++ ["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
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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)
Copy link
Collaborator

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 🤔

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator

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)
Copy link
Collaborator

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?

.github/workflows/test.yml Show resolved Hide resolved
@michaelpj
Copy link
Collaborator

I'd be happy to merge this as is and do improvements in follow ups, WDYT @fendor ?

@fendor
Copy link
Collaborator

fendor commented Nov 5, 2022

2-3 improvements are still missing, @VeryMilkyJoe is very responsive and will incorporate the requested changes until tomorrow.

@michaelpj
Copy link
Collaborator

There's not any hurry, I just thought you both might like to have it in :)

Copy link
Collaborator

@fendor fendor left a 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 :)

Jana Chadt and others added 4 commits November 10, 2022 14:57
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add plugin to format .cabal files
9 participants