Elm-format: Reduce VCS Diff Noise when introducing a `let`?

[rtfeldman]

The Problem

Consider viewEntries from TodoMVC. (No need to read all of it; we'll go over particular lines next.)

viewEntries : String -> List Entry -> Html Msg
viewEntries visibility entries =
    let
        isVisible todo =
            case visibility of
                "Completed" ->
                    todo.completed

                "Active" ->
                    not todo.completed

                _ ->
                    True

        allCompleted =
            List.all .completed entries

        cssVisibility =
            if List.isEmpty entries then
                "hidden"
            else
                "visible"
    in
        section
            [ class "main"
            , style [ ( "visibility", cssVisibility ) ]
            ]
            [ input
                [ class "toggle-all"
                , type_ "checkbox"
                , name "toggle"
                , checked allCompleted
                , onClick (CheckAll (not allCompleted))
                ]
                []
            , label
                [ for "toggle-all" ]
                [ text "Mark all as complete" ]
            , Keyed.ul [ class "todo-list" ] <|
                List.map viewKeyedEntry (List.filter isVisible entries)
            ]

Today viewEntries looks like this, but let's rewind time to a hypothetical earlier implementation...one where we had not yet introduced that let binding:

viewEntries : String -> List Entry -> Html Msg
viewEntries visibility entries =
    section
        [ class "main" ]
        [ input
            [ class "toggle-all"
            , type_ "checkbox"
            , name "toggle"
            ]
            []
        , label
            [ for "toggle-all" ]
            [ text "Mark all as complete" ]
        , Keyed.ul [ class "todo-list" ] <|
            List.map viewKeyedEntry entries
        ]

We commit this to version control. Then later, we add the allCompleted functionality like so:

viewEntries : String -> List Entry -> Html Msg
viewEntries visibility entries =
    let
        allCompleted =
            List.all .completed entries
    in
        section
            [ class "main" ]
            [ input
                [ class "toggle-all"
                , type_ "checkbox"
                , name "toggle"
                , checked allCompleted
                , onClick (CheckAll (not allCompleted))
                ]
                []
            , label
                [ for "toggle-all" ]
                [ text "Mark all as complete" ]
            , Keyed.ul [ class "todo-list" ] <|
                List.map viewKeyedEntry entries
            ]

Here's what we did:

1. Add allCompleted in a let binding.
2. Add checked allCompleted to the input.
3. Add onClick (CheckAll (not allCompleted)) to the input.

Really minor change.

However, here's the VCS diff of this change:

 viewEntries : String -> List Entry -> Html Msg
 viewEntries visibility entries =
-    section
-        [ class "main" ]
-        [ input
-            [ class "toggle-all"
-            , type_ "checkbox"
-            , name "toggle"
+    let
+        allCompleted =
+            List.all .completed entries
+    in
+        section
+            [ class "main" ]
+            [ input
+                [ class "toggle-all"
+                , type_ "checkbox"
+                , name "toggle"
+                , checked allCompleted
+                , onClick (CheckAll (not allCompleted))
+                ]
+                []
+            , label
+                [ for "toggle-all" ]
+                [ text "Mark all as complete" ]
+            , Keyed.ul [ class "todo-list" ] <|
+                List.map viewKeyedEntry entries
             ]
-            []
-        , label
-            [ for "toggle-all" ]
-            [ text "Mark all as complete" ]
-        , Keyed.ul [ class "todo-list" ] <|
-            List.map viewKeyedEntry entries
-        ]

This happened because we had to indent everything after in - creating a flood of VCS diff noise that make it impossible to tell which line(s) in the Html actually changed. This diff is useless.

This can be even worse when we want to add a let to an update function. We have to indent the entire case-expression, even if we're only modifying a branch or two. The diff is even more gigantic and useless.

I've run into this on a fairly regular basis in Elm, and it's been a source of periodic frustration. Considering Elm's style guide already has rules that exist primarily to dodge less impactful VCS diff problems (for example, putting a newline after every foo = to avoid VCS diffs caused by switching from single-line to multiline), this seems worth addressing.

Alternative Formattings

There are a few ways that style guide changes could resolve this VCS diff issue.

One would be to have elm-format indent let-expressions differently from how it indents everything else. However, introducing the inconsistency of "half-indents for let-expressions only" seems like an undesirable hack to have in Elm's style guide. Beginners might report it as a bug in elm-format.

Another would be to preemptively "double indent" areas like function bodies that are prone to wanting let at a later time:

viewEntries visibility entries =
        section
            [ class "main" ]
            ...

Beginners would almost certainly report this as a bug in elm-format. Assuming it were adopted, this approach would raise the question of whether branches of if and case expressions should also get double-indented, since they are also prone to wanting let at a later time. There might be other candidates as well.

Here's another way:

viewEntries visibility entries =
    let
        allCompleted =
            List.all .completed entries 
    in section
        [ class "main" ]
        ...

This is a "less bad" approach; the VCS diff would include 1 spurious line (in section) rather than N spurious lines.

Proposed Solution

The most consistent solution seems to be formatting it like this:

viewEntries visibility entries =
    let
        allCompleted =
            List.all .completed entries 
    in
    section
        [ class "main" ]
        ...

Here's the VCS diff from this version:

 viewEntries : String -> List Entry -> Html Msg
 viewEntries visibility entries =
+    let
+        allCompleted =
+            List.all .completed entries
+    in
     section
         [ class "main" ]
         [ input
             [ class "toggle-all"
             , type_ "checkbox"
             , name "toggle"
+            , checked allCompleted
+            , onClick (CheckAll (not allCompleted))
             ]
             []
         , label

Much clearer!

Here's how TodoMVC would look if reformatted in this style.

Thoughts?

[egauderman]

I think this is a good proposal. I'll add that I think this is also good for people who don't use elm-format.

If you're manually indenting and un-indenting, it's a lot of work to have to select and indent/unindent a huge block if you're adding and removing let-bindings while iterating quickly.

[tekul]

Has indenting let and in by two spaces also been considered as an option (similar to the way where is often used in Haskell)? This would allow both code blocks to be indented by the same standard four spaces.

Possible advantages I can see over the proposal:

• It will look closer to the existing style where both let and in blocks are indented by the same amount.
• Not being forced to indent by 8 spaces for the whole let block. Since this often has a lot more code in it (multiple expressions and possibly function definitions), it would be nice not to have to wasting this space unnecessarily.

Update: Ooops. I see it was actually mentioned above, and in #346. I don't see it as being any more of a hack though, than indenting by 8 after a let and 4 after an in.

[avh4]

Implemented in f51590b1...b9d179ac

[danny-andrews]

Seems like we're hyper optimizing for diff readability, to the detriment of source readability...the style guide states "Goal: a consistent style that is easy to read and produces clean diffs." I think that order is important:

1. Easy to read.
2. Produces clean diffs.

This change makes code more difficult to read (or at least inconsistent).

---

Sorry for noise on a closed issue. Just couldn't help myself.

[ehamberg]

(Since this still has the “discussion” label… 😉)

Before putting too much weight on diff readability, one should also consider that diff tools – including the git command line and GitHub – can be told to ignore white-space changes, which is great for the exact scenario mentioned above where on has white-space changes mixed with “real” changes.

This is how the change in elm-format diff looks when _showing_ white-space changes:

https://github.com/avh4/elm-format/compare/f51590b1dad6b5bf167952af0f8ec3ef81f56ba8...b9d179ac759d784e0125f2de694e863323f3489e

… and this is how it looks when _ignoring_ whitespace changes:

https://github.com/avh4/elm-format/compare/f51590b1dad6b5bf167952af0f8ec3ef81f56ba8...b9d179ac759d784e0125f2de694e863323f3489e?w=1

[rtfeldman]

This change makes code more difficult to read (or at least inconsistent).

How so?

[danny-andrews]

foo x =
  let
    y = -- Why do I indent after `let` keyword
      x + 1
  in
  y + 2 -- But not `in`? What makes it so special?

[rtfeldman]

There has never been a one-size-fits-all rule for indentation after keywords. For example, we indent right after let and else but not right after if and case.

There is a clear upside to having in in the "do not indent" group: diffing. I don't understand what the upside would be to putting it in the "indent" group instead.

[danny-andrews]

we indent right after let and else but not right after if and case.

In those cases, the difference is in whether or not you add a line break, not in whether or not you indent. The analogy here would be...

if
key == 40 then
else
    n

case
maybe of
  Just xs -> xs
  Nothing -> []

which I think you'll agree looks ugly. It might be a different story if I could keep the in expression on the same line, but elm-format moves it down (which is a whole other can of worms).

Before:

foo x =
  let
    y = x + 1
  in y + 2

After:

foo x =
    let
        y =
            x + 1
    in
    y + 2

[rtfeldman]

let
    foo

if
    foo

case
    foo

else
    foo

in
    foo

All of these are syntactically valid. elm-format indents some of them this way, and others it indents differently.

I understand that you think it looks better with the value after in indented.

My point is that there is nothing inconsistent about formatting it this way. There have always been different rules for how indentation does or does not happen after different keywords.

[danny-andrews]

I think you missed my point. In the case of if and case elm-format doesn't indent at all. In fact, it will remove indentation, if there exists any, in order to put what follows on the same line as the keyword. And if it did indent, I would expect it do to it like this:

if
  key == 40 then

rather than this:

if
key == 40 then

My point is: any time elm-format breaks a line of code onto a newline directly after a keyword, it indents one level.

elm-format output:

then
  foo

else
  foo

of
  foo

let
  foo

in
foo -- wtf!? Even GitHub highlights it differently. :D

If you can show me a counterexample, then I'll agree "there is nothing inconsistent about formatting it this way."

[danny-andrews]

One last note: optimizing for diff readability in Elm really seems like a waste of effort. One of the main selling-points of Elm is that you can be confident when you change code. In Tereza's talk she talks about a refactor she did across 100 files. I highly doubt the person who reviewed that PR scoured every line of the diff output.

If there is ever the slightest conflict between source readability and diff readability (as in this case) source readability should win.

[Mousaka]

If it's worth it or not I'm not sure but I second @danny-andrews point that it does seem to be inconsistent with how we format other things.

[avh4]

I've just read through the recent message here, and I'm not sure what's being discussed. This issue is closed because it was implemented. If there's a new issue to discuss, please create a new issue for it and and give a clear example of what change you're proposing. Thanks!