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 fourmolu plugin (attempt 2) and add Brittany for ghc-8.10.1 #264

Merged
merged 14 commits into from
Aug 8, 2020

Conversation

georgefst
Copy link
Collaborator

Git shenanigans on #161 did not quite do what I hoped. A fresh PR seems cleaner.

@georgefst georgefst mentioned this pull request Aug 1, 2020
@georgefst
Copy link
Collaborator Author

Not sure what to do about Stack.

It seems that the allow-newer option isn't as fine-grained as in Cabal, so this opens up a can of worms. Especially as it also seems, confusingly, to "allow older"...

@jneira
Copy link
Member

jneira commented Aug 1, 2020

I try to avoid allow-newer as much I can too.
Could you list what packages force us to use it? A comment with that list above the allow-newer annotation would be useful, to try to remove it asap

@georgefst
Copy link
Collaborator Author

georgefst commented Aug 1, 2020

Could you list what packages force us to use it?

See cabal.project in the diff. It might be worth listing them in the stack.yamls as well, I guess. I'm watching the two relevant PRs and intending to come back here and remove allow-newer as soon as it's possible.

@georgefst
Copy link
Collaborator Author

It looks like the remaining CI failures aren't just down to Stack anyway - there's a dependency conflict on GHC 8.6.5 with Cabal as well. I've hopefully just fixed that.

@georgefst
Copy link
Collaborator Author

I've hopefully just fixed that.

Well... Apparently not.

Which, actually, I should have realised, since 8.8 was already building with Cabal but not Stack. Perhaps we still need more extra-deps... I haven't really got time to look in to this again today. 😢

@lukel97
Copy link
Collaborator

lukel97 commented Aug 3, 2020

Argh this is really unfortunate. What we might have to do is vendor floskell+stylish haskell in the stack.yamls by just pointing them to our fork with the fixed bounds for now

@georgefst
Copy link
Collaborator Author

stylish-haskell has now actually been fixed as well. So I'm inclined to wait a few days and see if the floskell maintainer responds to my request for a Hackage upload.

Anyway, I've learnt my lesson here: don't use bleeding-edge features of a library when intending to integrate with a large project...

@georgefst
Copy link
Collaborator Author

I'm confused by some of the outputs from Stack here...

For example, where is ghcide-0.2.0 -> base-compat-batteries-0.10.5 coming from?

@lukel97
Copy link
Collaborator

lukel97 commented Aug 6, 2020

Hopefully we can get this into 0.3.0.0 as well #272
Trying to build the stack-8.8.x versions to see what's happening

@lukel97
Copy link
Collaborator

lukel97 commented Aug 6, 2020

The Brittany failures are coming in now because it's now a manual flag #250, and presumably with aeson-1.5.0.0 Brittany was just silently not being built because its bounds are too restrictive. Time to chase up Brittany now I guess...

Edit I see you've already beat me to it lspitzner/brittany#311

@georgefst
Copy link
Collaborator Author

Hopefully we can get this into 0.3.0.0 as well #272

Would be nice. I'd forgotten about Brittany though (since it's disabled in HLS with GHC 8.10), so we would need to depend on that via Github rather than Hackage. lspitzner/brittany#311

@lukel97
Copy link
Collaborator

lukel97 commented Aug 6, 2020

I think Lennart Spitzner might be AWOL at the moment, so I'm happy enough if we vendor it, since its looking like we'll have to anyway with #288
Can you add the GitHub version to both the cabal.project and stack.yamls?

@georgefst
Copy link
Collaborator Author

@bubba There are still Stack issues unfortunately. I'd be very grateful if anyone who's more familiar with it (and so isn't just cargo-culting extra-depss) could take a look.

@georgefst
Copy link
Collaborator Author

Argh. Cabal now failing due to haskell/cabal#5444.

I'm beginning to think this PR is cursed.

@lukel97
Copy link
Collaborator

lukel97 commented Aug 6, 2020

Argh then I think we need to vendor lspitzner/brittany#305 as well, which needs updated so that it builds on both ghc-8.10.1 and below
Surely this is what they mean by cabal hell

@lukel97
Copy link
Collaborator

lukel97 commented Aug 6, 2020

I'm going to try and patch lspitzner/brittany#305 now, then rebase it to include the aeson bounds bump

@lukel97
Copy link
Collaborator

lukel97 commented Aug 7, 2020

Ok tests are failing now because of an lsp-test bug which I've fixed in lukel97/lsp-test@23b1dcf, will make a release for it and then pull that in. Other than that I've also emailed the hackage trustees about getting a base bounds revision for data-tree-print. But I wouldn't let that block this PR again, we can drop the allow-newers whenever that revision is made

Fixes bug with document versions in testing
@lukel97 lukel97 changed the title Add fourmolu plugin (attempt 2) Add fourmolu plugin (attempt 2) and add Brittany for ghc-8.10.1 Aug 7, 2020
@lukel97 lukel97 requested review from jneira, fendor and alanz August 7, 2020 12:48
@georgefst
Copy link
Collaborator Author

Thanks for getting this over the line @bubba!

When is the HLS release due? We are quite likely to make some changes in the next few days that majorly affect the default formatting style (e.g. leading commas). So if these aren't done in time, it might be worth labelling the Fourmolu support a "preview" in the release notes.

@georgefst
Copy link
Collaborator Author

it might be worth labelling the Fourmolu support a "preview"

There's also the fact that documentation of the configuration options is currently rather lacking. Which, again, I intend to fix ASAP.

@lukel97
Copy link
Collaborator

lukel97 commented Aug 7, 2020

I think we'll make a release once this is in, and sure we can label the version of each of the formatters so the users aren't surprised when they changed. I want to wait on the thumbs up from someone else just to make sure they're cool with the allow-newer for this one version

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

@lukel97 lukel97 merged commit d74d111 into haskell:master Aug 8, 2020
@lukel97
Copy link
Collaborator

lukel97 commented Aug 8, 2020

@georgefst merged, you have the patience of a saint

@georgefst
Copy link
Collaborator Author

I've just been thinking about this, and the current state of things would result in a fairly poor first impression for people who would stumble across Fourmolu via the HLS settings. In particular, nested lists and records and up with a weird mix of two and four space indentation, which VScode detects as using two spaces.

This finally getting merged has prompted me to start working on these issues. So it'll all be a lot more polished in a month, in time for the next HLS release.

Therefore, what might actually be best is just to hold off on merging haskell/vscode-haskell#240. That way the functionality is there, but only for people who go looking for it.

@georgefst
Copy link
Collaborator Author

Therefore, what might actually be best is just to hold off on merging haskell/vscode-haskell#240.

Although, of course, that's entirely VScode-centric... Do the other editor integrations even use a drop down menu for formatting options, or do they just expect the user to enter a valid string?

@lukel97
Copy link
Collaborator

lukel97 commented Aug 10, 2020

@georgefst depends on the editor, but yeah the editor needs to manually provide dropdown options, there's nothing in the spec to provide discovery within a configuration file

pepeiborra pushed a commit that referenced this pull request Dec 27, 2020
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.

None yet

4 participants