tweag / ormolu

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

Instance declarations are double-spaced. #431

Closed cdsmith closed 4 years ago

cdsmith commented 4 years ago

This: https://code.world/haskell#P1YYfE2tmTPYIMJBKh7rHlw becomes this: https://code.world/haskell#P71SxA1-aRo3lISoy75cv1w

The result is many times less readable. This isn't even a question of inconsistent spacing. The original made a perfectly consistent choice.

neongreen commented 4 years ago

This is tricky. What would be the right way to format the instance if one of the methods had two equations?

instance Num a => Num (Diff a) where
  D u dudx + D v dvdx = D (u + v) (dudx + dvdx)
  D u dudx - D v dvdx = D (u - v) (dudx - dvdx)
  D u dudx * D v dvdx = D (u * v) (u * dvdx + v * dudx)
  negate (D u dudx) = D (-u) (-dudx)
  negate (Z u dudx) = ...
  abs (D u _) = D (abs u) (signum u)
  signum (D u _) = D (signum u) 0
  fromInteger n = D (fromInteger n) 0

I assume we would want to put blank lines around it:

instance Num a => Num (Diff a) where
  D u dudx + D v dvdx = D (u + v) (dudx + dvdx)
  D u dudx - D v dvdx = D (u - v) (dudx - dvdx)
  D u dudx * D v dvdx = D (u * v) (u * dvdx + v * dudx)

  negate (D u dudx) = D (-u) (-dudx)
  negate (Z u dudx) = ...

  abs (D u _) = D (abs u) (signum u)
  signum (D u _) = D (signum u) 0
  fromInteger n = D (fromInteger n) 0

But then this has the potential of being quite jarring if the instance is a mix of single-line and multi-line methods.

An alternative is "add blank lines everywhere if even one method is a multi-line method", which I guess fares better, but we'd have to be sure that having no blank lines is always more readable, because we would (probably?) also remove blank lines even if the author put them in.

Or we might just preserve all blank lines, see #74.

cdsmith commented 4 years ago

From an outsider perspective, it seems obvious that #74 is the right answer. I could also see a rule that multiline statements must be surrounded by blank lines (with an exception, I suppose, for a type annotation for a single symbol followed immediately by the definition of that symbol), but that seems optional to me.

I definitely don't think you want to force removal of blank lines, just like you don't want to force inserting them. I was attracted to Ormolu precisely because it seemed to be charting a pragmatic path where users could make high-level decisions about presentation, and formatting would just follow their lead.

neongreen commented 4 years ago

@mrkkrp OK, so what do you think? Would you accept a PR that would make Ormolu respect the blank lines / lack of them in method declarations? (with normalization of multiple blank lines to one, of course)

neongreen commented 4 years ago

s/in method declarations/between method declarations/

mrkkrp commented 4 years ago

The question for me is whether we should try to use existing blank lines or should we just introduce a more complex logic that decides to insert blank lines. The second option would be more normalizing I guess.

neongreen commented 4 years ago

Sure – but I can't think of a piece of logic that could do that without messing up badly in some cases, and we want a Haskell formatter that would be suitable for different kinds of code, not just for the type of code you and I tend to write.

E.g. I have never written a Num instance like the one in OP's example in my life. So I wouldn't feel comfortable designing a normalization rule without community input, and while I don't have this input I would go with "little or no normalization" as opposed to "some kind of normalization that works well for me".

Hence I think "preserve blank lines" is a good rule to stick to, similar to the "multi-line vs single-line layout" rule Ormolu has already adopted.

mrkkrp commented 4 years ago

@neongreen If you like, you can go ahead with preserving blank lines between class methods when there were blank lines in the input. That means that if there are two methods without blank lines between them they should stay "together" otherwise let's insert one blank line.

Ciantic commented 4 years ago

Does this also mean that there will be no blank line after the instance declaration?

I hope I'm not spamming, but this is unreadable for me at the moment:

data Foo a = Foo a

data Boo a = Boo a

instance Functor Foo where
  fmap f foo = undefined

instance Applicative Foo where

  pure x = undefined

  f <*> foo = undefined

instance Functor Boo where
  fmap f foo = undefined

instance Applicative Boo where

  pure x = undefined

  f <*> foo = undefined

Only suggestions I have is that don't add blanks lines between declarations if they are single line constructs, and never add extra blank line after where.

mrkkrp commented 4 years ago

Only suggestions I have is that don't add blanks lines between declarations if they are single line constructs

This is not the same as what I proposed. I think the idea with following the user's original decisions in this case is a better one and it is also more flexible.

never add extra blank line after where

I'm not against this.

srid commented 4 years ago

I'll take a stab at implementing this.

mrkkrp commented 4 years ago

@srid Please go ahead, it looks like @neongreen is either busy or lost interest.

neongreen commented 4 years ago

Hence why I asked @srid to implement this 😉