Consistently format code

m-renaud commented 6 years ago

The formatting of the code is incredibly inconsistent throughout this package, even within the same file. A few example:

No consistent column limit (some lines as long as 109 chars).

Where function body starts (also applies to case statements)

name args = body
name args =
body
name args =
body
name args
= body

Alignment of where clause. Usually indented 2 from function name, but sometimes 4
Where clause definitions on same line or hanging
Indentation of case statement (anywhere from 2 to 4 spaces)
```
case x of
a ->
a ->
a ->
```

This is a tiny bit of a bikeshed but having this much inconsistency hurts readability imho. Agreeing on some common indentation scheme and then defining Brittany rules for it would be really useful for code health/quality.

m-renaud commented 6 years ago

I'm personally a huge fan of https://gist.github.com/evancz/0a1f3717c92fe71702be, its main focus is on readability and minimizing diffs when things change.

treeowl commented 6 years ago

I'm not a big fan of those rules. For example, I often prefer restricted imports (import Foo (x)) to qualified ones, but it depends on the particular situation. And I don't really think selecting a type-specific mapping function is really the best way to keep track of what its argument means; good variable names and comments are more effective. I haven't read through the whole thing, but those are general first impressions. I do think we should impose a general rule that almost all non-qualified imports should be restricted. The exceptions are for utterly boring modules like Data.Map.Lazy, and probably also test modules importing the modules they're supposed to test.

treeowl commented 6 years ago

Also, remember: someone has to actually do a ton of work to fix up code to match a style guide, and then everyone in the future needs to learn and enforce it. Is it worth the trouble? I'm not sure.

m-renaud commented 6 years ago

Oh sorry, I should have been more specific, I was just referring to the indentation rules there; I was in a bit of a rush and just posted the link. I also fully support restricted imports and agree with the exceptions you mentioned at the end. Let me be a bit more specific:

Line length

try to stay within 80 (or 100) columns, don't care very strongly though

Type definitions

Sum types start constructors on the line following the declaration

Each constructor on its own line

data Boolean
= Literal Bool
| Not Boolean
| And Boolean Boolean
| Or Boolean Boolean
deriving (Show)

Record constructor on same line, fields start on the following line and use leading commas
```
data Circle = Circle
{ x :: Float
, y :: Float
, radius :: Float
}
deriving (Show, Eq)
```

Function definitions

body always starts on new line (exception: do)
indented by 2 spaces
where indented 1 spaces, declaration indented 1 more

foo a b = do
  this <- foo a b
  let that =
    if someThingIsTrueAbout this then
      goWithThat1
    else
      goWithThat2
  go this that

 where
  go x y = 
    case x of
      A ->
        doSomethingWith y
      B b x' ->
        frobnicate b <> go x' y

someone has to actually do a ton of work to fix up code to match a style guide, and then everyone in the future needs to learn and enforce it

All of this can be done automatically with a formatter (Brittany appears to be the best one and is actively developed).

Is it worth the trouble?

I'm not 100% convinced, but if can be automated, why not? Also, I just wanted to start the conversation to see what people's thoughts are :)

Note: I'm not trying to change the whole codebase around, just wanted to see how people feel about introducing a style guide :)

treeowl commented 6 years ago

I agree that line lengths below 80 are generally preferable, but there are also occasions when that rule will just make an even bigger mess (likely for the append code in Data.Sequence). The formatting of type declarations may depend on circumstances. The rules in the style guide seem to be designed for types that may change a little without too much other code changing. I don't believe that is the case for any of the types in containers, so I don't know that it really makes sense to worry about it. I generally prefer vertical formatting for more than a couple constructors/fields, but if that's all we have, one line seems fine.

I generally prefer a minimum of two spaces of indentation. That includes two spaces before the where and two spaces after. Why is do a special exception to the function body rule? Is the thought that a function that begins with do will probably always begin with do? The general rule seems sensible for "large" functions, but a bit obnoxious for tiny (especially local) ones.

I see you end a line above with then; I don't approve. I believe an if/then/else should either sit on one line or have one line that begins with then and one that begins with else. Should then and else be indented? I don't have strong feelings; it's prettier, and necessary in do blocks, but it uses precious horizontal space.

I'm the sort who tends to get annoyed by style rules (the more rigid the worse), so I will take a bit of convincing. That doesn't mean I can't be convinced, especially if you can demonstrate that automation leads to good results.

m-renaud commented 6 years ago

below 80...occasions when that rule will just make an even bigger mess

Agreed, I don't think 80 should be a hard limit. There are a bunch of examples where it goes over 80 when it doesn't need to though.

https://github.com/haskell/containers/blob/master/Data/IntSet/Internal.hs#L775 is just one.

two spaces before the where and two spaces after

Sure, I don't have a strong preference, the following looks just as good to me:

foo a b =
  go a b
  where
    go x y = undefined

Why is do a special exception to the function body rule?

Err, I don't have a good reason other than it would be weird if the body of monadic functions had indentation different than 2(?). If you bring the do down then you have:

foo =
  do thing1
     thing2

or

foo =
  do
    thing1
    thing2

Which looking at it seems fine to me, but every Haskell auto formatter that I've seen follows the style of having the do on the same line and the body of the do hanging.

I see you end a line above with then; I don't approve

That's fair, this alternative is fine with me too, although it looks kinda weird when you have a then on its own line.

if someThingIsTrueAbout this
then
  goWithThat1 onItsOwnLineBecauseItsTooLongToBeOnThenSameAsThen
else
  goWithThat2 sameLogicAppliesHere

I guess all that I really want is some internal consistency. Here are some examples of inconsistencies within the same file:

IntSet/Internal.hs

106 chars unnecessarily: https://github.com/haskell/containers/blob/master/Data/IntSet/Internal.hs#L775

Equals on next line after function name: https://github.com/haskell/containers/blob/master/Data/IntSet/Internal.hs#L664-L665

Equals on same line as function name https://github.com/haskell/containers/blob/master/Data/IntSet/Internal.hs#L700-L701

intset-properties.hs

Same inconsistencies as in IntSet/Internal.hs, in addition:

3 space indent https://github.com/haskell/containers/blob/master/tests/intset-properties.hs#L80

2 spaces after = in function definition before function body https://github.com/haskell/containers/blob/master/tests/intset-properties.hs#L183

IMHO with a style guide its easier to read the code (which is what you do 90% of the time) because your brain isn't constantly tripped up on the small line to line/function to function changes in formatting. Its especially noticeable when you have one function that uses 2 space indents and then another that uses 4, when looking at the 4 space indent function your brain does a double take thinking "is there an if or case statement I'm in?" (This also applies to hanging the -> in a case statement). When the formatting is consistent its easier to focus on the content.

treeowl commented 6 years ago

I meant

if someThingIsTrueAbout this
  then goWithThat1
         onItsOwnLineBecauseItsTooLongToBeOnThenSameAsThen
  else goWithThat2 sameLogicAppliesHere

haskell / containers