Skip to content

Bump Fourmolu to 0.4 #2254

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

Merged
merged 13 commits into from
Oct 5, 2021
Merged

Bump Fourmolu to 0.4 #2254

merged 13 commits into from
Oct 5, 2021

Conversation

georgefst
Copy link
Collaborator

@georgefst georgefst commented Oct 4, 2021

Addresses the relevant checklist item in #297.

import Data.Bifunctor (first)
import qualified Data.Text as T
import Development.IDE hiding (pluginHandlers)
import Development.IDE.GHC.Compat as Compat hiding (Cpp)
import qualified Development.IDE.GHC.Compat.Util as S
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how this file was passing CI before? Seeing as this line causes stylish-haskell to reformat all the other imports (I personally detest this behaviour, but that's an argument for another day).

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

Choose a reason for hiding this comment

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

Ideally, I'd remove 0.3 entirely, as 0.4 includes some important bug fixes. But this isn't possible while stylish-haskell is tied to older versions of GHC and Cabal.

(Same applies to Ormolu here - right now, the latest version will never be chosen by the dependency solver)

Copy link
Collaborator Author

@georgefst georgefst Oct 4, 2021

Choose a reason for hiding this comment

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

the latest version will never be chosen by the dependency solver

Actually, looks like this only applies to GHC < 9. The stylish-haskell plugin is disabled for 9.0.1.

And if CI passes here for GHC 9.0.1, we know that Fourmolu 0.4 is being used, since it's the first version to support it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe adding a comment about, to remove 0.3 some day?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe adding a comment about, to remove 0.3 some day?

Actually, I think there's no need. I'll probably end up at least trying to remove it every time I bump the upper bound, and eventually it'll be possible.

@jneira
Copy link
Member

jneira commented Oct 4, 2021

@georgefst many thanks to spend time on fourmolu/fourmolu#104 and this, getting support for ghc-9

@jneira
Copy link
Member

jneira commented Oct 4, 2021

I hope you dont mind i enabled fourmolu for ghc-9 to give it a shot in ci

@georgefst
Copy link
Collaborator Author

georgefst commented Oct 4, 2021

@georgefst many thanks to spend time on fourmolu/fourmolu#104 and this, getting support for ghc-9

Thanks, but @brandonchinn178 has actually done most of the hard work recently.

And in fact, as for GHC 9 support, the work was done upstream in Ormolu.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm i hope fourmolu will continue rocking in the formatter side

@jneira jneira mentioned this pull request Oct 4, 2021
35 tasks
@jneira
Copy link
Member

jneira commented Oct 4, 2021

There is a build error for ghc-9:

 src/Ide/Plugin/Fourmolu.hs:20:1: error:
Error:     Could not find module ‘GhcPlugins’
    Perhaps you meant GHC.Plugins (from ghc-9.0.1)
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
   |
20 | import           GhcPlugins                      (HscEnv (hsc_dflags))
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@georgefst
Copy link
Collaborator Author

There is a build error for ghc-9:

 src/Ide/Plugin/Fourmolu.hs:20:1: error:
Error:     Could not find module ‘GhcPlugins’
    Perhaps you meant GHC.Plugins (from ghc-9.0.1)
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
   |
20 | import           GhcPlugins                      (HscEnv (hsc_dflags))
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That import should have been removed in ec53fcb. Fixed in 46d3264.

This would have been caught by -Wall, but for some reason that isn't enabled for many of the plugins?

@jneira
Copy link
Member

jneira commented Oct 4, 2021

This would have been caught by -Wall, but for some reason that isn't enabled for many of the plugins?

Jumm they should afaiu, maybe is cargo culting and otoh it is enabled for hls itself, maybe it is time to give a try at least here

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

what about ghc-options: -Wall -Wredundant-constraints, like hls itself?

(without -Wno-name-shadowing -Wno-unticked-promoted-constructors if this does not need it)

@georgefst
Copy link
Collaborator Author

georgefst commented Oct 4, 2021

-Wredundant-constraints

Meh, I think there are legitimate use cases for redundant constraints, though it's very unlikely to come up in this plugin anyway.

-Wno-name-shadowing

I prefer to be warned.

-Wno-unticked-promoted-constructors

No strong opinion, but again unlikely to come up.

On the whole, I'd go for keeping things simple with just -Wall. But I don't feel that strongly about any of it.

@jneira
Copy link
Member

jneira commented Oct 4, 2021

-Wredundant-constraints

Meh, I think there are legitimate use cases for redundant constraints, though it's very unlikely to come up in this plugin anyway.

-Wno-name-shadowing

I prefer to be warned.

-Wno-unticked-promoted-constructors

No strong opinion, but again unlikely to come up.

On the whole, I'd go for keeping things simple with just -Wall. But I don't feel that strongly about any of it.

Agree with you, i forgot to mention -Wincomplete-uni-patterns (it is in ghcide but no hls, we have to unify all flags for sure 😓)

@georgefst
Copy link
Collaborator Author

it is in ghcide but no hls, we have to unify all flags for sure 😓

Yeah, the fact that I know of no good way to do this in a multi-cabal-package project (dhall-to-cabal with flags etc. defined in a single file? probably a totally uncharted path and almost certainly more effort than it's worth right now) is why I suggest keeping it simple.

@jneira jneira added the merge me Label to trigger pull request merge label Oct 4, 2021
@georgefst
Copy link
Collaborator Author

CI test failure looks unrelated to Fourmolu...

@jneira
Copy link
Member

jneira commented Oct 4, 2021

CI test failure looks unrelated to Fourmolu...

Yeah, all pr's are affected by #2253 😢

@georgefst
Copy link
Collaborator Author

CI test failure looks unrelated to Fourmolu...

Probably due to dependency changes? This PR does bump index-state considerably.

@georgefst
Copy link
Collaborator Author

CI test failure looks unrelated to Fourmolu...

Yeah, all pr's are affected by #2253 😢

Ah, ok.

@jneira
Copy link
Member

jneira commented Oct 5, 2021

CI test failure looks unrelated to Fourmolu...

Yeah, all pr's are affected by #2253 cry

Ah, ok.

I was wrong, not all pr's are affected only those which are updating the cabal.project index state so maybe a new dep version or the cache invalidation is triggering it

@mergify mergify bot merged commit ee4b496 into haskell:master Oct 5, 2021
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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants