Add fourmolu plugin #161

georgefst · 2020-06-14T14:14:48Z

There's an unfortunate amount of code duplication: Fourmolu.hs only has trivial changes from Ormolu.hs, but I don't think there's any sane way to abstract much of that out.

georgefst · 2020-06-14T14:17:55Z

Actually, this had better not go in until it also supports stack. I believe Matt was going to upload to Stackage once we've fully resolved fourmolu/fourmolu#1.

fendor · 2020-06-14T14:31:10Z

Actually, this had better not go in until it also supports stack

shouldnt it just work as an extra-dep?

georgefst · 2020-06-14T14:34:45Z

shouldnt it just work as an extra-dep?

Oh, probably, yeah. I haven't touched stack in a few years - I've forgotten how that sort of thing works exactly.

jneira · 2020-06-14T14:38:40Z

Mmm, maybe only if ormolu would let configure the number of spaces for identantion...
To have to duplicate an entire lib only for change that is... weird

georgefst · 2020-06-14T15:41:36Z

To have to duplicate an entire lib only for change that is... weird

Hard to disagree with that. The ormolu team are pretty insistent about the no-config thing though. And there seem to be quite a few of us who think it's a fantastic tool, but find the two spaces issue (and one or two others - in future there will likely be slightly more divergence, but not much) a deal-breaker. It's explained and discussed here.

Having both as dependencies here, and building almost exactly the same code twice, does seem particularly silly. I'd love HLS to have a more dynamic, 'pick-and-mix', plugin architecture (I brieflly discussed this earlier with @alanz), but that doesn't seem likely to happen particularly soon.

Edit: the plugin issue is now discussed at #164.

fendor · 2020-06-14T20:01:57Z

To have to duplicate an entire lib only for change that is... weird

At least it adds no additional dependencies 😇

jneira · 2020-06-14T20:40:44Z

Well, i would replace ourmolu with fourmolu if the later would let configure the number of spaces.
Maybe i will create another almighty fork that let users choose them 😆

ndmitchell · 2020-06-15T09:15:34Z

I think fourmolu has a ticket to add configuration of the number of spaces.

georgefst · 2020-06-17T17:10:42Z

I think fourmolu has a ticket to add configuration of the number of spaces.

Yeah, I'm working on that now.

Once that's done, should I open a fresh PR which uses fourmolu in place of ormolu?

Some existing users might be confused if ormolu has gone and they now need to write an explicit configuration file to get its behaviour back. But then it would also be confusing if we had fourmolu using two spaces by default...

ndmitchell · 2020-06-17T22:37:28Z

haskell-language-server has always been very free with allowing lots of different Haskell tools to coexist and the user gets to pick. Given Ormulu is popular, it should remain available as a separate option.

georgefst · 2020-06-17T22:53:44Z

Given Ormulu is popular, it should remain available as a separate option.

Sorry, I'm confused - are you now saying we should have both? I thought the consensus here was shifting the other way e.g. from @jneira's comments. And the question of Fourmolu's configurability isn't as relevant if both are in HLS.

I personally don't mind either way.

jneira · 2020-06-18T05:12:47Z

Well, i am still thinking include two formatters libs that only differ in one literal is really weird from a pure technical poitn of view. And more if one of them let you behave like the other one.

What will happen if one update its dependencies before the other one and both are incompatibles? We will have the responsability of make both compile with the same configuration.

That said, i dont have a good solution to @georgefst and @ndmitchell points so i will not be against to link both.

ndmitchell · 2020-06-18T12:55:05Z

I think having two formatters that differ by one literal is a bit mad. But the Haskell community does, and that's separate from HLS.

Once you have those two formatters, I don't think HLS should be the ones to pick between them - in all other aspects we're delightfully non-picky - Cabal/Stack/Nix/Obelisk/Bash or whatever. I don't see any reason to start here.

Note that all the concerns of compatible libraries etc apply equally to other libraries - e.g. we can't statically link two things that require different versions of haskell-src-exts etc. I suspect an eventual solution will be to demand that everything we use as a plugin is in Stackage nightly, and then that guarantees there is a solution that works for all.

georgefst · 2020-06-18T17:42:49Z

Well in that case this can go in as-is.

I'll come back and bump the version once we release 0.1 on Hackage, which will have some basic configurability and a minor bugfix, plus upstream changes. Probably within the next week.

georgefst · 2020-06-23T11:35:22Z

Well, I've got this building locally with Fourmolu 0.1, but we require the latest Aeson, so I've ended up needing:

allow-newer:
  cabal-plan:aeson
  stylish-haskell:aeson
  HsYAML-aeson:aeson
  floskell:aeson

The changes in 1.5 seem pretty unlikely to have broken anything, and there's a bugfix I'd rather not have to work around, hence the restrictive lower bound.

I assume there's a good reason we're using a fork of cabal-plan (the latest on Hackage already has the relaxed upper bound on Aeson)?

I could go and chase up the maintainers for the other three.

lukel97 · 2020-07-01T11:32:02Z

Looks like cabal-plan should be ok https://hackage.haskell.org/package/cabal-plan for that version of aeson
haskell/stylish-haskell#289

lukel97 · 2020-07-01T11:37:27Z

Also blocking haskell-hvr/HsYAML-aeson#6

lukel97 · 2020-07-01T11:43:16Z

ennocramer/floskell#49

src/Ide/Plugin/Fourmolu.hs

georgefst · 2020-07-02T11:00:37Z

src/Ide/Plugin/Fourmolu.hs

+        ex = map (("-X" <>) . show') $ S.toList $ D.extensionFlags df
+        show' x = case show x of
+          "Cpp" -> "CPP"
+          s -> s


This'll do for now. Really, we shouldn't have to rely on show here anyway...

It actually looks like it wouldn't take much of a refactoring to allow passing the DynFlags directly. I'll see if I can get this fixed upstream in Ormolu.

It actually looks like it wouldn't take much of a refactoring to allow passing the DynFlags directly.

Turns out to be slightly tricky because of the ghc/ghc-lib type mismatch. I'll be curious to see how @jneira solves this issue for HLint.

georgefst · 2020-07-02T11:03:08Z

cabal.project

+allow-newer:
+  floskell:aeson
+  HsYAML-aeson:aeson
+  stylish-haskell:aeson


We probably want to remove these before merging.

I've got an eye on the relevant PRs - thanks @bubba for opening them where necessary.

What are the remaining packages blocked by aeson-1.5?

Floskell got the bump, but there hasn't been a Hackage release with it.

We're still waiting on Stylish.

lukel97 · 2020-07-17T15:36:10Z

Ok looks like we might need to go down the hackage trustee route haskell-infra/hackage-trustees#271

Seeing as, since haskell#209, the README suggests globally enabling `-haddock`, it's rather unfortunate for that not to work with HLS' own installation script...

Fix haddock parse error in install.hs

The original lineno extracted from the Addition DiffOperation needs to be increased by 1.

We were taking max of l and sl but the position of an insertion in the newer document is irrelevant since the edits will be applied from bottom to top.

lukel97

Did you mean to update the ghcide submodule? Other than that I think we can live with the allow-newers for now, this PR looks great and it's been collecting dust here for a while now

lukel97 · 2020-07-31T10:34:00Z

test/functional/Format.hs

+        let formatLspConfig provider =
+                object [ "languageServerHaskell" .= object ["formattingProvider" .= (provider :: Value)] ]


Already defined on lines 172-173

Suggested change

let formatLspConfig provider =

object [ "languageServerHaskell" .= object ["formattingProvider" .= (provider :: Value)] ]

lukel97 · 2020-07-31T10:56:25Z

We should also probably incorporate the fix from #246 into this as well. Either with this PR or in a separate one

Hoogle nor happy nor cabal-helper shouldn't be needed anymore

georgefst · 2020-07-31T11:19:39Z

I'll try and sort this this evening, if we're happy to leave the two allow-newers in for now.

The final two issues appear to be the result of some fairly careless rebasing on my part...

georgefst · 2020-07-31T12:31:49Z

It would actually be preferable for #246 and #257 to get merged first.

I can't see that there's anything holding either of them up?

Ormolu flags

Ormolu fix

Use @? and @?= instead, since they give much easier to read error messages. The `shouldX` functions all just end up dumping the Show instance of a HSpecFailure or what have you which is really hard to read. It also doesn't look like hspec-expectations is that actively maintained anymore

…-steps Remove redundant CircleCI steps

Fixes flakey CI builds. Turns out it was trying to run every single test in parallel at once, which is why when haskell#247 added two extra tests it was just enough to push it over the limit and cause things to fail

Slow down Tasty by limiting it to -j1

Remove hspec-expectations

Remove a redundant caching step

georgefst · 2020-08-01T00:30:55Z

Closing in favour of #264.

Experience shows that people sometimes mistakenly start `ghcide` on the command line with the `--lsp` option (which is intended to be used only in server/client communication scenarios) and then wonder why nothing is working.. So let's issue a warning message whenever `--lsp` is used.

This was referenced Jun 16, 2020

Stop operators from being indented past do. fourmolu/fourmolu#6

Merged

Documentation fourmolu/fourmolu#7

Closed

georgefst mentioned this pull request Jun 18, 2020

Add Fourmolu as a formatting provider haskell/vscode-haskell#240

Merged

georgefst force-pushed the master branch from 39830f7 to 0fa07ae Compare June 23, 2020 09:24

lukel97 mentioned this pull request Jul 1, 2020

Allow aeson 1.5 haskell-hvr/HsYAML-aeson#6

Merged

georgefst mentioned this pull request Jul 1, 2020

Use cabal-plan from Hackage #185

Merged

georgefst force-pushed the master branch from 7853251 to 7a08e1b Compare July 2, 2020 10:42

georgefst commented Jul 2, 2020

View reviewed changes

src/Ide/Plugin/Fourmolu.hs Outdated Show resolved Hide resolved

georgefst commented Jul 2, 2020

View reviewed changes

georgefst mentioned this pull request Jul 10, 2020

Fix stack.yaml (aeson-1.5.2.0 dependency). fourmolu/fourmolu#14

Merged

georgefst and others added 9 commits July 29, 2020 13:59

Fix haddock parse error in install.hs

605d19a

Seeing as, since haskell#209, the README suggests globally enabling `-haddock`, it's rather unfortunate for that not to work with HLS' own installation script...

Merge pull request haskell#255 from georgefst/patch-1

4fe0f7d

Fix haddock parse error in install.hs

Add more tests for checking formatting

3a6874d

Add more tests for ormolu

96d42f3

Fix bug in diffOperationToTextEdit

d4371fd

The original lineno extracted from the Addition DiffOperation needs to be increased by 1.

Fix another bug in diffOperationToTextEdit

75f0d6b

We were taking max of l and sl but the position of an insertion in the newer document is irrelevant since the edits will be applied from bottom to top.

Don't normalize in ormolu

679e0ec

Ormolu RegionIndices should be 1-based

f51d4e8

Remove *.ormolu.unchanged formatting tests

7a8f51d

lukel97 approved these changes Jul 31, 2020

View reviewed changes

Remove redundant CircleCI steps

5dbf153

Hoogle nor happy nor cabal-helper shouldn't be needed anymore

fendor and others added 12 commits July 31, 2020 14:44

Merge pull request haskell#246 from pepeiborra/ormolu

530ccb0

Ormolu flags

Merge pull request haskell#257 from sureyeaah/ormolu-fix

5163c41

Ormolu fix

Merge pull request haskell#259 from haskell/remove-redundant-circleci…

3dbfa68

…-steps Remove redundant CircleCI steps

Slow down Tasty by limiting it to -j1

38672e0

Fixes flakey CI builds. Turns out it was trying to run every single test in parallel at once, which is why when haskell#247 added two extra tests it was just enough to push it over the limit and cause things to fail

Merge pull request haskell#261 from haskell/fix-ci

bd5e256

Slow down Tasty by limiting it to -j1

Remove a redundant caching step

26f1e7f

Merge pull request haskell#260 from bubba/remove-hspec-expectations

0adf4bd

Remove hspec-expectations

Merge pull request haskell#262 from Ailrun/fix-ci

75d4c62

Remove a redundant caching step

Fix compression extension on GitHub build artifacts

a308151

Add fourmolu plugin

d961197

Merge branch 'master' into fourmolu

e0ea2fe

georgefst mentioned this pull request Aug 1, 2020

Add fourmolu plugin (attempt 2) and add Brittany for ghc-8.10.1 #264

Merged

georgefst closed this Aug 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fourmolu plugin #161

Add fourmolu plugin #161

georgefst commented Jun 14, 2020

georgefst commented Jun 14, 2020

fendor commented Jun 14, 2020

georgefst commented Jun 14, 2020

jneira commented Jun 14, 2020

georgefst commented Jun 14, 2020 •

edited

Loading

fendor commented Jun 14, 2020

jneira commented Jun 14, 2020

ndmitchell commented Jun 15, 2020

georgefst commented Jun 17, 2020

ndmitchell commented Jun 17, 2020

georgefst commented Jun 17, 2020

jneira commented Jun 18, 2020

ndmitchell commented Jun 18, 2020

georgefst commented Jun 18, 2020

georgefst commented Jun 23, 2020

lukel97 commented Jul 1, 2020

lukel97 commented Jul 1, 2020

lukel97 commented Jul 1, 2020

georgefst Jul 2, 2020

georgefst Jul 17, 2020

georgefst Jul 2, 2020

lukel97 Jul 31, 2020

georgefst Jul 31, 2020

lukel97 commented Jul 17, 2020

lukel97 left a comment

lukel97 Jul 31, 2020

lukel97 commented Jul 31, 2020

georgefst commented Jul 31, 2020

georgefst commented Jul 31, 2020

georgefst commented Aug 1, 2020

		let formatLspConfig provider =
		object [ "languageServerHaskell" .= object ["formattingProvider" .= (provider :: Value)] ]

Add fourmolu plugin #161

Add fourmolu plugin #161

Conversation

georgefst commented Jun 14, 2020

georgefst commented Jun 14, 2020

fendor commented Jun 14, 2020

georgefst commented Jun 14, 2020

jneira commented Jun 14, 2020

georgefst commented Jun 14, 2020 • edited Loading

fendor commented Jun 14, 2020

jneira commented Jun 14, 2020

ndmitchell commented Jun 15, 2020

georgefst commented Jun 17, 2020

ndmitchell commented Jun 17, 2020

georgefst commented Jun 17, 2020

jneira commented Jun 18, 2020

ndmitchell commented Jun 18, 2020

georgefst commented Jun 18, 2020

georgefst commented Jun 23, 2020

lukel97 commented Jul 1, 2020

lukel97 commented Jul 1, 2020

lukel97 commented Jul 1, 2020

georgefst Jul 2, 2020

Choose a reason for hiding this comment

georgefst Jul 17, 2020

Choose a reason for hiding this comment

georgefst Jul 2, 2020

Choose a reason for hiding this comment

lukel97 Jul 31, 2020

Choose a reason for hiding this comment

georgefst Jul 31, 2020

Choose a reason for hiding this comment

lukel97 commented Jul 17, 2020

lukel97 left a comment

Choose a reason for hiding this comment

lukel97 Jul 31, 2020

Choose a reason for hiding this comment

lukel97 commented Jul 31, 2020

georgefst commented Jul 31, 2020

georgefst commented Jul 31, 2020

georgefst commented Aug 1, 2020

georgefst commented Jun 14, 2020 •

edited

Loading