The newlines-between-decls=2 test failures might be an actual bug. Could someone take a look?

This looks great at a glance! Let me know when it's ready for review.

Ha, great timing!

The newlines-between-decls=2 test failures might be an actual bug. Could someone take a look?

Yeah, I expected this testing would reveal some bugs.

CC @wraithm, who authored #48.

I'm here. Investigating. I see those test failures! Could someone recap what's going on? That's a big diff 😂 Is this PR basically pulling in a bunch of tests from upstream? I would assume that the issue is that the test vectors aren't taking my code into account.

The idea of this PR is to test all fourmolu's options in isolation, as opposed to all together. "Testing" here is twofold: for fixed test vectors, we (1) format them and see that AST remains the same, and (2) commit the formatting results to repo for regression testing. The latter accounts for the large diff.

So the vectors are the same, and indeed pass with newlines-between-decls=1. It's newlines-between-decls=2 that fails.

Those should produce different output, ie. The test vectors should be different. I'll write here what they should be for the 2 case.

In this case, it's not merely that the output doesn't match what's expected. There's a more serious issue.

The log shows an OrmoluASTDiffers error for three of the test cases (involving magic "disable" comments and CPP), which means that the output parses differently to the input.

This isn't hugely surprising - this PR is finally tackling the testing hole I mentioned way back in #48 (comment).

Ok, this is a subtle and not-particularly-interesting bug, and I've run out of time for now.

I've put some progress on this branch, in case anyone wishes to take a look. It essentially consists of:

• Setting up for GHCID feedback, as outlined in the docs.
• Printing some extra info at the error source.

@wldhx Could you rebase your branch on to master? Should be trivial.

Ok, this is a subtle and not-particularly-interesting bug, and I've run out of time for now.

I've put some progress on this branch, in case anyone wishes to take a look. It essentially consists of:

* Setting up for GHCID feedback, as outlined [in the docs](https://github.com/fourmolu/fourmolu/blob/master/DEVELOPER.md#instant-feedback-with-ghcid).

* Printing some extra info at the error source.

The following are minimal reproducing inputs for the two (related) issues:

{-# LANGUAGE CPP #-}

x = do
#ifdef XXXXXXXX
#endif
  ()

{- ORMOLU_DISABLE -}
{- ORMOLU_ENABLE -}
x = ()

Done. I suggest we squash on merge.

Tests should pass if you cherry-pick ed241f9.

@wraithm Are you happy with the test output changes in that commit? It's basically just now not inserting a blank line at the top of every file (and after magic comments and CPP). Was that behaviour intentional?

Which branch should I use to test?

You can use that exact commit actually. None of the other stuff on that branch will affect the output.

@georgefst I tried this:

$ git checkout ed241f9020ebb353cb85f2e3c3e39071902892b2 
$ stack build
fourmolu> build (lib + exe)
Preprocessing library for fourmolu-0.4.0.0..
Building library for fourmolu-0.4.0.0..
[43 of 49] Compiling Ormolu.Parser.Result

src/Ormolu/Parser/Result.hs:43:8: error: [-Wunused-matches, -Werror=unused-matches]
    Defined but not used: ‘pr’
   |
43 |   show pr = "PR"
   |        ^^

Is that right? I don't think I have anything unusual about my setup. Perhaps just that I'm using stack here?

I just turned off -Werror, and that got it to build. I ran fourmolu against our codebase, and everything looked great!

(I saw two changes. 1. There's no longer a half indent for additional patterns in pattern guards. 2. Haddock comments on record fields got changed from -- | above the field decl to -- ^ below the field decl. Is there a setting to configure that now? I don't mind that change, btw, but I might want to add the setting to our fourmolu.yaml.)

Is that right? I don't think I have anything unusual about my setup. Perhaps just that I'm using stack here?

Ah, I don't use Stack and didn't realise we had -Werror enabled, which seems like a bad idea for local development. @brandonchinn178 Is there an easy way to enable it on CI only?

1. There's no longer a half indent for additional patterns in pattern guards. 2. Haddock comments on record fields got changed from -- | above the field decl to -- ^ below the field decl.

These are both intentional improvements: see and tweag/ormolu#798 and #124, resp. If you really want the old behaviour, we could discuss having a flag.

@wldhx Sorry that this thread is getting a little off-topic. I'll review once we have tests passing.

Ah, I don't use Stack and didn't realise we had -Werror enabled, which seems like a bad idea for local development. @brandonchinn178 Is there an easy way to enable it on CI only?

I actually intentionally want -Werror on in local development. If it's gonna fail in CI, why not have it fail locally? cabal.project also has it nominally on (although it should be updated to fourmolu +dev instead of ormolu +dev), so I'd rather we change it so both Stack and Cabal have -Werror on in local development

Tests should pass if you cherry-pick ed241f9.

@georgefst I missed that at the time, sorry! Tests are green now.

ci/circleci: test_ghc_9.2 Expected — Waiting for status to be reported

Is something stuck?..

You need to rebase against master and rerun tests (and fix conflicts too)

@brandonchinn178 Done.

the -Werror issue is being merged over here: #139

@georgefst can you finish reviewing this when you get a chance? I don't feel qualified enough to ship this on my own

What's the relation between this PR, #49, and #132 ?

132 an 129 both implement 49. IMO 129 supersedes 132 by having option bundles.

Thanks @dpwiz. To elaborate some more, both this PR and #132 address the need for more tests as raised by #49. Here's how they differ:

• This PR tests both separate options and bundles
• This PR generates separate options' labels with TH
• Support test sets for printer options #132 adds a knob to disable idempotence check

@georgefst @brandonchinn178 Could we give getting this in another try? I'd really love to see all the features currently blocked on tests merged.

I'd really love to see all the features currently blocked on tests merged.

So would I! I have some time off next week, and hope to get to this.

Your explanation of the relationship between this and #132 is helpful, thanks. I'm curious whether @gustavoavena sees it the same way?

Apologies again for how long it's taking. I've been pretty swamped for months.

Could we perhaps commit to a date we want something resolving #49 merged? I'm not particularly attached to this PR; #132 would work just as well to unblock feature merge.

@georgefst @brandonchinn178 Just another gentle ping on this.

@georgefst @brandonchinn178 Is there any way I could help to get this merged? I understand you're pretty busy so I'd love to take on some of the work. :)

I added an update to #49. To reiterate what I said in that issue, many thanks for the work you've put into this PR. In light of the new direction, though, I'll be closing this PR for now. Please let me know if you have any questions, comments, or concerns. Thanks!