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

Support fourmolu ^>= 0.7 #2944

Merged
merged 4 commits into from
Jun 11, 2022
Merged

Conversation

parsonsmatt
Copy link
Contributor

@parsonsmatt parsonsmatt commented Jun 8, 2022

This PR adds support for fourmolu-0.7

@parsonsmatt parsonsmatt requested a review from georgefst as a code owner June 8, 2022 16:08
Comment on lines +12 to +13
homepage: https://github.com/haskell/haskell-language-server
bug-reports: https://github.com/haskell/haskell-language-server/issues
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took me a minute to figure out the Git repository for this package, so I added these fields to the .cabal file. Technically an unrelated change, would be easy to put somewhere else.

@@ -1,9 +1,10 @@
{-# LANGUAGE CPP #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to provide backwards compatibility.

@@ -1,9 +1,10 @@
{-# LANGUAGE CPP #-}
{-# LANGUAGE DataKinds #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extension is reshuffled because I ran stylish-haskell (which appears to be how this file is formatted) and it automatically did that change.

Same for the reordered imports below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So stylish-haskell isn't enforced by CI? If not, I'd be tempted to go back to fourmolu.

This file was formatted with stylish-haskell when it looked like that was becoming necessary (#693, #1439). But I lost track of where the decision on formatting this codebase ended up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a pre-commit hook that runs stylish-haskell, mentioned in the contributing guidelines. But it is not enforced by CI:

### Formatter pre-commit hook
We are using [pre-commit-hook.nix](https://github.com/cachix/pre-commit-hooks.nix) to configure git pre-commit hook for formatting. Although it is possible to run formatting manually, we recommend you to use it to set pre-commit hook as our CI checks pre-commit hook is applied or not.
You can configure the pre-commit-hook by running
``` bash
nix-shell
```
If you don't want to use [nix](https://nixos.org/guides/install-nix.html), you can instead use [pre-commit](https://pre-commit.com) with the following config.
```json
{
"repos": [
{
"hooks": [
{
"entry": "stylish-haskell --inplace",
"exclude": "(^Setup.hs$|test/testdata/.*$|test/data/.*$|test/manual/lhs/.*$|^hie-compat/.*$|^plugins/hls-tactics-plugin/.*$|^ghcide/src/Development/IDE/GHC/Compat.hs$|^ghcide/src/Development/IDE/Plugin/CodeAction/ExactPrint.hs$|^ghcide/src/Development/IDE/GHC/Compat/Core.hs$|^ghcide/src/Development/IDE/Spans/Pragmas.hs$|^ghcide/src/Development/IDE/LSP/Outline.hs$|^plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs$|^ghcide/test/exe/Main.hs$|ghcide/src/Development/IDE/Core/Rules.hs|^hls-test-utils/src/Test/Hls/Util.hs$)",
"files": "\\.l?hs$",
"id": "stylish-haskell",
"language": "system",
"name": "stylish-haskell",
"pass_filenames": true,
"types": [
"file"
]
}
],
"repo": "local"
},
{
"repo": "https://github.com/pre-commit/pre-commit-hooks",
"rev": "v4.1.0",
"hooks": [
{
"id": "mixed-line-ending",
"args": ["--fix", "lf"],
"exclude": "test/testdata/.*CRLF*.hs$"
}
]
}
]

import Ide.Types
import Language.LSP.Server hiding (defaultConfig)
import Language.LSP.Types
import Language.LSP.Types.Lens (HasTabSize (tabSize))
import Ormolu
import Ormolu.Config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is redundant for fourmolu <= 0.7. I didn't gate it behind CPP because CPP is ugly.

Copy link
Collaborator

@georgefst georgefst left a comment

Choose a reason for hiding this comment

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

Commit history is a bit weird, but this otherwise looks good!

library
exposed-modules: Ide.Plugin.Fourmolu
hs-source-dirs: src
ghc-options: -Wall
build-depends:
, base >=4.12 && <5
, filepath
, fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.6
, fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.6 || ^>= 0.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is in the wrong commit, right? Not necessarily actually worth changing now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am regrettably a complete git noob and generally just squash+merge all my stuff 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, I'm very much in the "git is a wonderful concept with a horrible CLI" camp. Since I finally got around to setting up good editor integration, I haven't looked back.

@@ -1,9 +1,10 @@
{-# LANGUAGE CPP #-}
{-# LANGUAGE DataKinds #-}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So stylish-haskell isn't enforced by CI? If not, I'd be tempted to go back to fourmolu.

This file was formatted with stylish-haskell when it looked like that was becoming necessary (#693, #1439). But I lost track of where the decision on formatting this codebase ended up.

@@ -29,7 +29,8 @@ library
build-depends:
, base >=4.12 && <5
, filepath
, fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.6 || ^>= 0.7
-- , fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.6 || ^>= 0.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Eugh, I take it we still can't drop support for old versions due to stylish-haskell: #2254 (comment)?

(if we could, we could obviously remove all the CPP)

Copy link
Collaborator

Choose a reason for hiding this comment

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

stylish-haskell has been updated since #2254 and I think it's very likely that this can be removed. Do you want to check in a follow-up PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #2950.

Copy link
Collaborator

@georgefst georgefst Jun 13, 2022

Choose a reason for hiding this comment

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

See #2950.

CI failed because we have a longer support window than I thought. So the commit was force-pushed away from that branch. See #2951 instead.

@July541 July541 mentioned this pull request Jun 11, 2022
@pepeiborra pepeiborra merged commit bb6b4e1 into haskell:master Jun 11, 2022
@July541
Copy link
Collaborator

July541 commented Jun 12, 2022

I checked the test detail and found all tests are run for fourmolu 0.3.0.0-0.6.0.0 :(

georgefst added a commit that referenced this pull request Jun 12, 2022
Some of these have genuine bugs, as discussed in #2254 (comment). We also get to drop a load of CPP introduced in #2944.

Users can still use older versions via CLI mode.

This wasn't possible until recently because `stylish-haskell`'s `ghc-lib-parser` bounds were holding us back.
@georgefst
Copy link
Collaborator

I checked the test detail and found all tests are run for fourmolu 0.3.0.0-0.6.0.0 :(

Yep, #2950 fixes the test, and optimistically drops support for old versions (lets see if that passes CI...).

georgefst added a commit that referenced this pull request Jun 12, 2022
Some of these have genuine bugs, as discussed in #2254 (comment). We also get to drop a load of CPP introduced in #2944.

Users can still use older versions via CLI mode.

This wasn't possible until recently because `stylish-haskell`'s `ghc-lib-parser` bounds were holding us back.
hololeap pushed a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
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.

4 participants