tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
938 stars 83 forks source link

Don't indent closing paren where possible #1101

Closed brandonchinn178 closed 1 month ago

brandonchinn178 commented 4 months ago

Currently, multiline tuples in function arguments look like this:

f
  ( a,
    b,
    c,
    ) = _

It would look a bit nicer if the closing paren was not indented like

f
  ( a,
    b,
    c,
  ) = _

This doesn't work in all cases; specifically, it's a syntax error to do this in case expressions. But we can do this in all of the places where it makes sense.

I had to do this anyway for Fourmolu (it looks even more egregious with leading commas), and figured I'd send a patch upstream, in case y'all want it.

amesgen commented 3 months ago

FTR: There are also other cases that work very similarly; if we go with this PR, we should probably do the same for them for consistency (not asking you to do that here :sweat_smile:).

Some examples, showing multi-line patterns as a regular argument, and in a \case pattern (where we need extra indentation for the closing part).

InputOrmolu 0.7.4.0 output
Lists ```haskell foo [ x ] = x bar = \case [ x ] -> x ``` ```haskell foo [ x ] = x bar = \case [ x ] -> x ```
Unboxed tuples ```haskell foo (# x #) = x bar = \case (# x #) -> x ``` ```haskell foo (# x #) = x bar = \case (# x #) -> x ```

While writing this, it seems clear that a small advantage of the current way of always adding the extra indentation to the closing bracket even when unnecessary is that you can refactor a "normal" pattern to e.g. a \case without being confused why your code suddenly doesn't parse anymore. However, I don't know how often this actually occurs, it might not be very relevant.

brandonchinn178 commented 3 months ago

Sure, it is nice to do the one thing that works in all cases. But there is precedence; I think we add some extra indentation specifically for do blocks and such, so it's not crazy to make that exception here too. Up to you

brandonchinn178 commented 1 month ago

@amesgen Any thoughts on merging this?

mrkkrp commented 1 month ago

I'm not sure about this one. It deviates from the status quo and makes the code more complex. Both effects are not exactly welcome per se but can be accepted if there is something in the change that makes it worth the trouble. So, it boils down to the question "how important/desirable is this change?"