Merge ormolu-0.4.0.0 by brandonchinn178 · Pull Request #138 · fourmolu/fourmolu

brandonchinn178 · 2021-11-29T20:16:46Z

Adds GHC 9.2 support to fourmolu (for free!)

Needs fixing (either in this PR or in a followup PR):

~~Fix indentation after guard~~
~~Fix import newline regression~~
Fix indentation after list

Release generation fails on MacOS 11 due to #804.

Brittany cannot format itself, but we can format it with Ormolu. It is a good idea to add it to our tests.

We have stopped doing that and I think it is nice for readability.

brandonchinn178 · 2021-11-29T20:18:55Z

data/examples/declaration/value/function/multiple-guards-four-out.hs

    | x == 6 =
-        foo x
-            + foo 20
+            foo x
+                + foo 20


This is due to an Ormolu fix that doesn't look good with 2-space indentation (it also didn't look good with Fourmolu's 2-space indentation). A later PR should probably edit the logic to say "if indentation is <= 2, add an indentation, otherwise, don't"

(tweag/ormolu#813)

Agreed on the fix. Perhaps we should tackle it in this PR if it's that simple? Otherwise we can open an issue, but we'll have to make sure we mark it high-priority and fix it before the next release.

If you're ok with it, I'd rather merge this PR asap and open a high priority issue after

We definitely need to fix this before making a release. My inclination is to fix it before merging, rather than letting the regression hit master. Why do you feel differently?

Same goes for the stray newlines in import lists.

(I was about to push a commit reverting the outputs to what we'd hope to see, and suggest that we then fix the tests properly in this PR. But I realised that would be rude without getting your opinion first.)

I was about to push a commit reverting the outputs to what we'd hope to see, and suggest that we then fix the tests properly in this PR.

See #140 instead for said changes.

Why do you feel differently?

Personally, I would prefer merging ormolu PRs to only resolve merge conflicts because of the sheer amount of changes that are being merged in. If I were to fix the stuff now, the PR diff would include both merge conflict changes (i.e. changes that are needed to merge anything at all) and "feature work" changes. Having the "feature work"/regression fixes commits be separate helps a bit, but in a PR where 20/30 commits are from upstream, the fourmolu-specific commits can get lost in the weeds.

But I don't feel too strongly. I can continue working on this branhc

Well, the separation is still visible as long as we don't squash-merge. I'd prefer that to breaking the main branch.

ormolu-live/README.md

brandonchinn178 · 2021-11-29T20:22:57Z

src/Ormolu/Parser.hs

+    (mkT dropBlankTypeHaddocks)
+    hsmod
+      { hsmodImports =
+          concat $ normalizeImports True (hsmodImports hsmod),


This function is new; Ormolu now automatically normalizes a module immediately after parsing. I'm forcing the parameter to normalizeImports here to True so that we can sort the imports, but not get rid of the raw grouping information so we can (optionally) print the groups separately later on depending on poRespectful

Could it be some error here which is leading to the unwanted blank lines in e.g. data/examples/import/sorted-four-out.hs?

Yeah, that makes sense to me

brandonchinn178 · 2021-11-29T20:24:51Z

src/Ormolu/Printer/Meat/Declaration/Data.hs

-            <> fmap getLoc con_ex_tvs
-            <> maybeToList (fmap getLoc con_mb_cxt)
+          [RealSrcSpan real Nothing | AddEpAnn AnnForall (EpaSpan real) <- epAnnAnns con_ext]
+          <> fmap getLocA con_ex_tvs


the deindent here looks weird. Ormolu has it correctly indented, but running fourmolu deindents it back. A latter PR should investigate and fix

brandonchinn178 · 2021-12-06T04:50:50Z

FYI @georgefst you'll need to go to Settings > Protected branches and edit the protection to remove 8.8 and add 9.2

brandonchinn178 · 2021-12-22T00:39:44Z

@georgefst this is blocking the aeson-2 upgrade in stackage, do you think you can review this by January?

georgefst · 2021-12-22T11:14:41Z

Thanks for letting me know! This has been near the top of my to-do list for a while, but that's probably the kick I needed. Fortunately I've just finished work for the holidays, so I should get to this in the next few days.

georgefst · 2022-01-08T20:47:41Z

Thanks for letting me know! This has been near the top of my to-do list for a while, but that's probably the kick I needed. Fortunately I've just finished work for the holidays, so I should get to this in the next few days.

Time flies. Got terribly sidetracked by the inability to build this on GHC 8.10 (digital-asset/ghc-lib#344). I promise that barring any serious emergency I'll have this reviewed within 24 hours.

I have been using this heavily via a fork of the ghc-9.2 branch of HLS for the last week, with just one small issue around reordering imports. So that's a good sign.

georgefst

Thanks for doing this! It's pretty thankless work and I really appreciate it.

data/examples/declaration/type-families/closed-type-family/multi-line-four-out.hs

georgefst · 2022-01-10T00:26:15Z

README.md

 If there are any options you'd like to see, let us know. If it's not too complicated to implement (and especially if you implement it yourself!) then we'll probably add it.

-Run `cabal test` and `./format.sh` before submitting any pull requests.
+See `DEVELOPER.md` for documentation.


I agree that this is no longer the right place for this.

But perhaps we should point out the option of running scripts/run-fourmolu.sh manually in DEVELOPER.md. Some people don't like pre-commit hooks.

Sure, will do

brandonchinn178 · 2022-01-10T01:52:51Z

data/examples/declaration/value/function/pattern/multiline-guard-statement-four-out.hs

-        5 -> True
-        _ -> False
+            5 -> True
+            _ -> False


FYI reverting the change to multiple-guard-statements-four-out.hs also reverts this change. This is correct and matches Ormolu's behavior

Wow, ok, looks weird to me but this is definitely not to place to change it then.

brandonchinn178 · 2022-01-10T01:59:09Z

Fixed DEVELOPER.md and the post-guard indent regression. Still working on the import regression

brandonchinn178 · 2022-01-10T02:22:23Z

Done. Should be ready to merge

georgefst · 2022-01-10T09:38:18Z

Nice! I'm happy to merge. Any idea what's holding up CI?

Do you want to handle the release? Otherwise I can do it this evening (GMT).

brandonchinn178 · 2022-01-10T16:03:25Z

#138 (comment)

FYI @georgefst you'll need to go to Settings > Protected branches and edit the protection to remove 8.8 and add 9.2

Feel free to handle the release

brandonchinn178 and others added 24 commits October 16, 2021 12:34

Control regeneration of test examples via an env variable

5548d63

Use macOS-10.15 for release generation for MacOS

a12da78

Release generation fails on MacOS 11 due to #804.

Workaround for macOS 11 issues

4dcf814

Force a space after -- in line comments

7d4e47b

Check Ormolu by running it on Brittany

048c55c

Brittany cannot format itself, but we can format it with Ormolu. It is a good idea to add it to our tests.

Store raw lines in an array for faster indexing

753a73c

Indent post-guard expression one level more

c222964

Implement support for Backpack

b889919

Include comment stream in debug output

8b24cc0

Add weeder to CI

79b7b63

Add Ormolu Live

1eec905

Remove blank Haddock comments

d3cf91e

Add blank lines between entries in the changelog

331bf40

We have stopped doing that and I think it is nice for readability.

Mention HLS in README

009ea19

Preserve rewrite rules that are never active

dae9d25

Add commit hash alias for Ormolu Live

e003e11

Switch to ghc-lib-parser-9.2

11cda4c

Remove duplicated info from the recent changelog entry

ebe1754

Use newer Closure Compiler

e67a56d

Use custom macdylibbundler

e813e57

Rebuild all GHCJS dependencies by upgrading haskell.nix

3781d37

Version bump (0.4.0.0)

807ee05

Merge ormolu-0.4.0.0

b7a3272

Follow up fixes

13e2f1f

brandonchinn178 requested a review from georgefst November 29, 2021 20:16

brandonchinn178 commented Nov 29, 2021

View reviewed changes

ormolu-live/README.md Show resolved Hide resolved

brandonchinn178 commented Nov 29, 2021

View reviewed changes

brandonchinn178 force-pushed the merge-ormolu branch from ab8a7c7 to e4d0a8a Compare November 29, 2021 23:19

brandonchinn178 added 9 commits November 30, 2021 12:25

Update supported GHC versions

31c89d1

Update snapshot dependencies

8362bac

Run fourmolu on source code

b938cd5

Regenerate test outputs

8ffde77

Remove redundant test outputs

9b645fb

Remove weeder.dhall

7befcb0

Remove format.sh

9001aa4

Fix pre-commit for building fourmolu with Cabal

3bc905d

Move Ormolu-0.3.1.0 changelog to correct place

6116f83

brandonchinn178 force-pushed the merge-ormolu branch from e4d0a8a to 6116f83 Compare November 30, 2021 20:25

brandonchinn178 mentioned this pull request Dec 1, 2021

[WIP] Support GHC 9.2 #136

Closed

brandonchinn178 requested a review from georgefst December 4, 2021 07:19

georgefst reviewed Jan 10, 2022

View reviewed changes

Update DEVELOPER.md

68fffea

brandonchinn178 commented Jan 10, 2022

View reviewed changes

Check indentation width for post-guard indents

69473d5

Don't normalize imports twice

274aba4

georgefst merged commit 4f37d73 into master Jan 10, 2022

brandonchinn178 deleted the merge-ormolu branch January 10, 2022 16:46

georgefst mentioned this pull request Jan 10, 2022

Merge Ormolu 0.4.0.0 #126

Closed

brandonchinn178 mentioned this pull request Jan 10, 2022

Regression with 0.5.0.0: Fix indentation after a list comprehension with infix operator #142

Closed

brandonchinn178 mentioned this pull request Apr 3, 2022

Regression: fourmolu-0.5.0.0 deindents multiline tuples #148

Closed

Conversation

brandonchinn178 commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonchinn178 commented Dec 6, 2021

Uh oh!

brandonchinn178 commented Dec 22, 2021

Uh oh!

georgefst commented Dec 22, 2021

Uh oh!

georgefst commented Jan 8, 2022

Uh oh!

georgefst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonchinn178 commented Jan 10, 2022

Uh oh!

brandonchinn178 commented Jan 10, 2022

Uh oh!

georgefst commented Jan 10, 2022

Uh oh!

brandonchinn178 commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brandonchinn178 commented Nov 29, 2021 •

edited

Loading

georgefst left a comment •

edited

Loading