-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Fix Fourmolu 0.7 support #2950
Fix Fourmolu 0.7 support #2950
Conversation
Fourmolu 0.7 (due to Ormolu 0.5) removes the `--cabal-default-extensions` flag, instead looking for Cabal files by default. If input is taken from stdin and `--stdin-input-file` isn't specified, it therefore fails. We don't need to make any changes in non-CLI mode because the Cabal file handling logic occurs at a higher level outside the entry point we use to the library (the `ormolu` function).
82edbca
to
0bbc545
Compare
@@ -66,7 +66,7 @@ provider plId ideState typ contents fp fo = withIndefiniteProgress title Cancell | |||
(exitCode, out, err) <- | |||
readCreateProcessWithExitCode | |||
( proc "fourmolu" $ | |||
["-d"] | |||
["-d", "--no-cabal"] |
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.
Well, here's a reason for the "compile against the package" approach: we don't have a sane way of knowing whether or not to pass this flag, since it depends on which CLI version the user has installed!
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.
we don't have a sane way of knowing whether or not to pass this flag
Sane or not, fixed in 581686c.
(CLI mode isn't enabled by default, so I'm not too concerned about any weirdness)
Allows us to use Fourmolu 0.7.
{ noCabal = v >= ["0", "7"] | ||
} | ||
Nothing -> do | ||
T.hPutStrLn stderr "couldn't get Fourmolu version" |
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.
This is bad error reporting, but I'm about to revamp the logging for the whole plugin anyway, since I've been told off by @fendor for printing directly to stdout
!
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.
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.
Are you going to tidy up the logging in this PR or later? Otherwise LGTM.
Later, but hopefully today. |
There are some weird things going on with CI. This PR appeared to be failing for a while, with some impenetrable error in one of the post-job tests. But that somehow sorted itself out without any intervention. And then, after merging, there's this failure. |
It seemed like it was stuck, so I restarted some jobs, that's why! |
Aha, thanks, I thought I was going mad. |
No description provided.