Skip to content

Add fourmolu as formatter #2735

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Add fourmolu as formatter #2735

wants to merge 7 commits into from

Conversation

rowanG077
Copy link
Member

@rowanG077 rowanG077 commented Jun 18, 2024

There are a ton of file which won't pass through formatting yet. The main reason is CPP.

To make this reviewable I intend to make a formatting prepare commit per sub-project where I try to fix it(or ignore that specific file if necessary). Then once that is done I will make a wholesale formatting commit. This is done to ensure the changes I made to make formatting work won't drown out in changes done by the formatter.

@rowanG077 rowanG077 force-pushed the formatter branch 5 times, most recently from 348bb5f to 18d8cb1 Compare June 18, 2024 19:47
@leonschoorl
Copy link
Member

Please exclude clash-ghc/src-bin-* from this.
Those file are basically copies of GHC's ghc/ghci binary with a little extra code added to handle the --[hdl] options and the :[hdl] commands in interactive mode.

When adding support for new GHC version to clash it is very helpful to have them changed as little as possible.

If there are actual problems with that code they should be fixed upstream.

@rowanG077 rowanG077 force-pushed the formatter branch 3 times, most recently from c2a673f to 1172c0b Compare June 18, 2024 20:55
@rowanG077 rowanG077 force-pushed the formatter branch 2 times, most recently from aebc056 to b79af65 Compare June 18, 2024 21:04
@DigitalBrains1
Copy link
Member

DigitalBrains1 commented Jun 19, 2024

Thanks for all the hard work; it's hardly fun but very useful. Some initial feedback.

I really like it if every commit passes CI. You first add the fourmolu formatting check to CI, and in a later commit make the existing code conform to that CI check. Could you reverse those so CI would also pass on the earlier commits?

Furthermore, you add LANGUAGE TemplateHaskell and LANGUAGE MagicHash to files in clash-testsuite, purely to make the file pass fourmolu. On the face of it, I think it would be better to adjust the fourmolu invocation for shouldwork and shouldfail so the extensions that clash enables by default for compilation are also enabled for fourmolu; you can apparently use -o or --ghc-opt. But actually, since these files are on occasion the literal reproducer sent to us by the people reporting an issue, I suggest we should just not invoke fourmolu on shouldwork and shouldfail. Someone might some day submit a reproducer that only works with a specific CPP incantation that fourmolu can't handle, where rewriting it to the basic form it can handle would make the reproducer stop working; sometimes very specific structures are needed to trip a certain compiler bug. We could work our way around it, by resolving the file to several versions without any CPP and then calling the correct file under the correct circumstances, but really, maybe those bug reproducers are not the code we want to make pretty anyway. I certainly apply a lot less rigour to writing those than to code that's part of Clash proper. It sounds like effort for very little gain.

@rowanG077
Copy link
Member Author

The goal of the commits is not to pass CI. It's to make fourmolu accept the file at all. To make CI pass I need to actually format the files and it will be the penultimate commit. Regarding formatting tests. I dislike adding those extensions wholesale to fourmolu since that means those extensions will be enabled for all other files as well. But I'm good with disable formatting for all tests as well.

What do you think @martijnbastiaan?

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented Jun 19, 2024

Ah, I think you misunderstood.

I like it if every commit could pass CI. But you add the fourmolu check in CI before you make the code pass that check. I'm saying that I'd rather see that you first make the code pass the check and only in a later commit add the check itself.

Because currently the commit which adds the fourmolu check will not pass CI. It's just an ordering thing, nothing has to change about the content.

If you were to currently force-push this branch to only contain the first commit Add fourmolu as formatter, the CI that runs for this PR would fail, because it would fail the added CI check. For the purposes of bisection, I really like it if any commit passes any check we throw at it.

@DigitalBrains1
Copy link
Member

Regarding formatting tests. I dislike adding those extensions wholesale to fourmolu since that means those extensions will be enabled for all other files as well.

I was suggesting making one step run fourmolu on all code except shouldwork and shouldfail, and adding another step running fourmolu on shouldwork and shouldfail, not run all code with the added options.

@martijnbastiaan
Copy link
Member

What do you think @martijnbastiaan?

Keeping yet another place with default extensions doesn't seem right to me, so I'd vote for disabling formatting for it entirely. I'm very unhappy with the current state of clash-testsuite anyway - the whole fact that non of these files are part of a cabal file makes it really hard to do general improvements like these. (I understand that some bugs are only triggerable by calling Clash on source files directly, but these are the exceptions, not the rule - that should be reflected in the testsuite structure.)

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