Configuration for minimal diffs (VCS friendly)

[hasufell]

I think many big projects try to minimize the amount of conflicts they have when merging, rebasing, etc etc, so the main goal is to create as little diff as possible.

One very important aspect of this is: never align identifiers based on other identifiers (except where it is necessary, e.g. for let)... because the layout changes too easily on code changes increasing the diff. Also see e.g. https://github.com/input-output-hk/adrestia/wiki/Coding-Standards#avoid-variable-length-indentation

There might be other things to do or avoid. Does anyone have ideas for an ultimate config for this?

[tfausak]

I typically use this as a starting point:

conf_layout:
  lconfig_columnAlignMode:
    tag: ColumnAlignModeDisabled
  lconfig_indentPolicy: IndentPolicyLeft

[hasufell]

I tried it and it leads to weird formatting wrt let and in. And there's also a bug in the list formatting.

Config

conf_layout:
  lconfig_columnAlignMode:
    tag: ColumnAlignModeDisabled
  lconfig_indentPolicy: IndentPolicyLeft

  lconfig_importColumn: 50
  lconfig_allowSingleLineExportList: true
  lconfig_importAsColumn: 50
  lconfig_hangingTypeSignature: true
  lconfig_indentListSpecial: true
  lconfig_indentAmount: 4
  lconfig_alignmentBreakOnMultiline: true
  lconfig_cols: 80
  lconfig_indentWhereSpecial: true

conf_forward:
  options_ghc:
  - -XBangPatterns
  - -XDataKinds
  - -XExplicitForAll
  - -XFlexibleContexts
  - -XFlexibleInstances
  - -XGADTs
  - -XImplicitParams
  - -XLambdaCase
  - -XMultiWayIf
  - -XPatternGuards
  - -XQuasiQuotes
  - -XRecursiveDo
  - -XTemplateHaskell
  - -XTupleSections
  - -XTypeFamilies
  - -XViewPatterns

Before

printProject :: PinGHC -> Project -> Maybe Text -> IO Text
printProject (PinGHC pin) (Project (Ghc ghc) pkgs srcs ghcOpts) hack = do
  ghcOpts' <- printGhcOpts ghcOpts
  pure $ T.concat $ [ "-- Generated by stackage-to-hackage\n\n"] <>
         withCompiler <>
         [ "packages:\n    ", packages, "\n\n"
         , sources, "\n"
         , "allow-older: *\n"
         , "allow-newer: *\n"
         ] <> ghcOpts' <> verbatim hack
  where
    withCompiler
      | pin = ["with-compiler: ", ghc, "\n\n"]
      | otherwise = []
    verbatim Nothing = []
    verbatim (Just txt) = ["\n-- Verbatim\n", txt]
    packages = T.intercalate "\n  , " (T.pack . addTrailingPathSeparator <$>
                                     NEL.toList pkgs)
    sources = T.intercalate "\n" (source =<< srcs)
    source Git{repo, commit, subdirs} =
      let base = T.concat [ "source-repository-package\n    "
                        , "type: git\n    "
                        , "location: ", repo, "\n    "
                        , "tag: ", commit, "\n"]
      in if null subdirs
         then [base]
         else (\d -> T.concat [base, "    subdir: ", d, "\n"]) <$> subdirs

    -- Get the ghc options. This requires IO, because we have to figure out
    -- the local package names.
    printGhcOpts (GhcOptions locals _ everything (PackageGhcOpts packagesGhcOpts)) = do
      -- locals are basically pkgs since cabal-install-3.4.0.0
      localsPrint <- case locals of
        Just x -> fmap concat $ forM pkgs $ \pkg -> do
          name <- fmap (unPackageName . pkgName) <$> getPackageIdent pkg
          pure $ maybe []
            (\n -> if M.member n $ M.mapKeys (unPackageName . pkgName . unPkgId)
                                             packagesGhcOpts
                   then []
                   else ["\npackage ", T.pack n, "\n    ", "flags: ", x, "\n"]
            )
            name
        Nothing -> pure []
      let everythingPrint = case everything of
            Just x -> ["\npackage ", "*", "\n    ", "flags: ", x, "\n"]
            Nothing -> []
      let pkgSpecificPrint = M.foldrWithKey
            (\k a b -> ["\npackage "
                       , (T.pack . unPackageName . pkgName . unPkgId $ k)
                       , "\n    "
                       , "flags: "
                       , a
                       , "\n"] <> b)
            [] packagesGhcOpts
      pure (everythingPrint <> localsPrint <> pkgSpecificPrint)

After

printProject :: PinGHC -> Project -> Maybe Text -> IO Text
printProject (PinGHC pin) (Project (Ghc ghc) pkgs srcs ghcOpts) hack = do
    ghcOpts' <- printGhcOpts ghcOpts
    pure
        $ T.concat
        $ ["-- Generated by stackage-to-hackage\n\n"]
        <> withCompiler
        <> [ "packages:\n    "
           , packages
           , "\n\n"
           , sources
           , "\n"
           , "allow-older: *\n"
           , "allow-newer: *\n"
           ]
        <> ghcOpts'
        <> verbatim hack
  where
    withCompiler
        | pin = ["with-compiler: ", ghc, "\n\n"]
        | otherwise = []
    verbatim Nothing = []
    verbatim (Just txt) = ["\n-- Verbatim\n", txt]
    packages = T.intercalate
        "\n  , "
        (T.pack . addTrailingPathSeparator <$> NEL.toList pkgs)
    sources = T.intercalate "\n" (source =<< srcs)
    source Git { repo, commit, subdirs } =
        let
            base = T.concat
                [ "source-repository-package\n    "
                , "type: git\n    "
                , "location: "
                , repo
                , "\n    "
                , "tag: "
                , commit
                , "\n"
                ]
        in
            if null subdirs
                then [base]
                else
                    (\d -> T.concat [base, "    subdir: ", d, "\n"]) <$> subdirs

    -- Get the ghc options. This requires IO, because we have to figure out
    -- the local package names.
    printGhcOpts (GhcOptions locals _ everything (PackageGhcOpts packagesGhcOpts))
        = do
      -- locals are basically pkgs since cabal-install-3.4.0.0
            localsPrint <- case locals of
                Just x -> fmap concat $ forM pkgs $ \pkg -> do
                    name <- fmap (unPackageName . pkgName)
                        <$> getPackageIdent pkg
                    pure $ maybe
                        []
                        (\n ->
                            if M.member n $ M.mapKeys
                                (unPackageName . pkgName . unPkgId)
                                packagesGhcOpts
                            then
                                []
                            else
                                [ "\npackage "
                                , T.pack n
                                , "\n    "
                                , "flags: "
                                , x
                                , "\n"
                                ]
                        )
                        name
                Nothing -> pure []
            let
                everythingPrint = case everything of
                    Just x -> ["\npackage ", "*", "\n    ", "flags: ", x, "\n"]
                    Nothing -> []
            let
                pkgSpecificPrint = M.foldrWithKey
                    (\k a b ->
                        [ "\npackage "
                            , (T.pack . unPackageName . pkgName . unPkgId $ k)
                            , "\n    "
                            , "flags: "
                            , a
                            , "\n"
                            ]
                            <> b
                    )
                    []
                    packagesGhcOpts
            pure (everythingPrint <> localsPrint <> pkgSpecificPrint)

[tfausak]

There's a lot to unpack there. Can you identify separate issues specifically?

[hasufell]

1

    pure
        $ T.concat
        $ ["-- Generated by stackage-to-hackage\n\n"]

why not

    pure $ T.concat
        $ ["-- Generated by stackage-to-hackage\n\n"]

More concise, wastes less space: again, aligning is a non-goal for this configuration.

2

        let
            base = T.concat
                [ "source-repository-package\n    "
                , "type: git\n    "
                , "location: "
                , repo
                , "\n    "
                , "tag: "
                , commit
                , "\n"
                ]
        in
            if null subdirs
                then [base]
                else
                    (\d -> T.concat [base, "    subdir: ", d, "\n"]) <$> subdirs

This is the worst imo, it should be:

        let base = T.concat
                [ "source-repository-package\n    "
                , "type: git\n    "
                , "location: "
                , repo
                , "\n    "
                , "tag: "
                , commit
                , "\n"
                ]
        in if null subdirs
                then [base]
                else (\d -> T.concat [base, "    subdir: ", d, "\n"]) <$> subdirs

• no newline after let
• no newline after in
• no newline after if, then else etc.

3

Same here

                            if M.member n $ M.mapKeys
                                (unPackageName . pkgName . unPkgId)
                                packagesGhcOpts
                            then
                                []
                            else
                                [ "\npackage "
                                , T.pack n
                                , "\n    "
                                , "flags: "
                                , x
                                , "\n"
                                ]

Should be

                            if M.member n $ M.mapKeys
                                (unPackageName . pkgName . unPkgId)
                                packagesGhcOpts
                            then []
                            else [ "\npackage "
                                , T.pack n
                                , "\n    "
                                , "flags: "
                                , x
                                , "\n"
                                ]

4

Aligning lists should be configurable, so that:

            base = T.concat
                [ "source-repository-package\n    "
                , "type: git\n    "
                , "location: "
                , repo
                , "\n    "
                , "tag: "
                , commit
                , "\n"
                ]

becomes

            base = T.concat
                [ "source-repository-package\n    " , "type: git\n    "
                , "location: " , repo , "\n    " , "tag: " , commit , "\n"
                ]

I don't think having one element per line really helps readability here. It's excessive.

5

            let
                pkgSpecificPrint = M.foldrWithKey
                    (\k a b ->
                        [ "\npackage "
                            , (T.pack . unPackageName . pkgName . unPkgId $ k)
                            , "\n    "
                            , "flags: "
                            , a
                            , "\n"
                            ]
                            <> b
                    )
                    []
                    packagesGhcOpts

The second item of the list is indented further, this looks like a bug.

[hasufell]

Another instance of excessive newlining:

Before

instance FromYAML Stack where
   parseYAML = withMap "Stack" $ \m -> Stack
       <$> m .: "resolver"
       <*> m .:? "compiler"
       <*> m .:? "packages" .!= mempty
       <*> m .:? "extra-deps" .!= mempty
       <*> m .:? "flags" .!= (Flags M.empty)
       <*> m .:? "ghc-options" .!= emptyGhcOptions

After:

instance FromYAML Stack where
    parseYAML = withMap "Stack" $ \m ->
        Stack <$> m .: "resolver"
            <*> m
            .:? "compiler"
            <*> m
            .:? "packages"
            .!= mempty
            <*> m
            .:? "extra-deps"
            .!= mempty
            <*> m
            .:? "flags"
            .!= (Flags M.empty)
            <*> m
            .:? "ghc-options"
            .!= emptyGhcOptions

This is unreadable.

[tfausak]

Thanks for taking the time to split all these out! Some of these look like bugs. Others of them look like things that could be controlled via configuration.

---

-- actual
foo
  $ bar
  $ baz "with some long argument over the line length"

-- expected
foo $ bar
  $ baz "with some long argument over the line length"

I personally prefer the way Brittany formats expressions like this currently. Once you bump over the line length each operator goes on its own line. It looks like you'd prefer a "paragraph fill" mode that puts as many parts of the expression on one line before breaking. Is that correct?

---

-- actual
let
  foo =
    bar "with some long argument over the line length"

-- expected
let foo =
      bar "with some long argument over the line length"

In this case I think Brittany is doing the right thing. If the layout is not supposed to depend on the length of any identifiers, adding a newline after let and indenting is the only thing it can reasonably do. Otherwise the layout will depend on the length of let. (In other words, everything would have to be indented four spaces rather than what the indentation is set at.)

---

-- actual
in
  foo

-- expected
in foo

I agree with you on this one. I've never understood why Brittany puts a newline immediately after in. Sounds like a bug!

---

-- actual
if foo
then bar
else
  baz "with some long argument over the line length"

-- expected
if foo
then bar
else baz
  "with some long argument over the line length"

-- or perhaps even
if foo then bar else baz
  "with some long argument over the line length"

This is similar to the last one. Seems like Brittany should try to put some tokens on the same line as else even when the whole expression is over the line length. It's unclear to me if the entire conditional expression should be collapsed as well.

---

-- actual
[ foo
, bar
, "long element over the line length"
]

-- expected
[ foo, bar
, "long element over the line length"
]

There is already an issue for this one: #302.

---

-- actual
(\x ->
  [ foo
    , "long element over the line length"
    ] <> x)

-- expected
(\x ->
  [ foo
  , "long element over the line length"
  ] <> x)

Agreed! Looks like another bug. Could be related to do notation somehow? See #296.

---

-- actual
foo
  <$> bar
  .: "baz"

-- expected
foo
  <$> bar .: "baz"

Unfortunately getting this one right requires knowing the precedence of the operators. See #79 for details.

---

There are a couple other "omnibus" issues that are similar to this one: #33 and #278.

[hasufell]

I personally prefer the way Brittany formats expressions like this currently. Once you bump over the line length each operator goes on its own line. It looks like you'd prefer a "paragraph fill" mode that puts as many parts of the expression on one line before breaking. Is that correct?

The problem is... it's a waste of vertical space and quickly becomes excessive, turning a line that exceeds the desired length by 2 chars into 10 line breaks. So yes, readability isn't always about increasing verbosity. It's also about making scrolling over code easier and spotting things. The more line breaks, the harder scrolling through code becomes, because everything is so noisy.

In this case I think Brittany is doing the right thing. If the layout is not supposed to depend on the length of any identifiers, adding a newline after let and indenting is the only thing it can reasonably do. Otherwise the layout will depend on the length of let. (In other words, everything would have to be indented four spaces rather than what the indentation is set at.)

I believe if the indenting is at least 4 spaces, then brittany can infer that it doesn't need a newline after =.

Unfortunately getting this one right requires knowing the precedence of the operators. See #79 for details.

I think this is very easy. The problem is that we're still trying to do layouting while we should just do indenting and careful newlining. If we only do the latter, this won't really be a problem. That's the entire point of this ticket and VCS friendly formatting: minimal changes, no layouting.

[tfausak]

It sounds like you may want a different tool entirely. Have you tried Ormolu? https://github.com/tweag/ormolu

[hasufell]

Have you tried Ormolu? https://github.com/tweag/ormolu

Yes and it's neither configurable, nor pleasant.