Configuration for minimal diffs (VCS friendly) #316

New Issue

2020-09-30T23:38:56+02:00

hasufell commented

2020-09-30 23:38:56 +02:00

(Migrated from github.com)

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?

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 commented

2020-10-01 04:03:10 +02:00

(Migrated from github.com)

I typically use this as a starting point:

conf_layout:
  lconfig_columnAlignMode:
    tag: ColumnAlignModeDisabled
  lconfig_indentPolicy: IndentPolicyLeft

I typically use this as a starting point: ``` yaml conf_layout: lconfig_columnAlignMode: tag: ColumnAlignModeDisabled lconfig_indentPolicy: IndentPolicyLeft ```

hasufell commented

2020-10-01 21:57:09 +02:00

(Migrated from github.com)

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)

I tried it and it leads to weird formatting wrt `let` and `in`. And there's also a bug in the list formatting. ## Config ```yaml 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 ```hs 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 ```hs 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 commented

2020-10-02 19:42:46 +02:00

(Migrated from github.com)

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

hasufell commented

2020-10-02 20:23:07 +02:00

(Migrated from github.com)

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.

## 1 ```hs pure $ T.concat $ ["-- Generated by stackage-to-hackage\n\n"] ``` why not ```hs 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 ```hs 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: ```hs 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 ```hs if M.member n $ M.mapKeys (unPackageName . pkgName . unPkgId) packagesGhcOpts then [] else [ "\npackage " , T.pack n , "\n " , "flags: " , x , "\n" ] ``` Should be ```hs 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: ```hs base = T.concat [ "source-repository-package\n " , "type: git\n " , "location: " , repo , "\n " , "tag: " , commit , "\n" ] ``` becomes ```hs 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 ```hs 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 commented

2020-10-03 19:55:23 +02:00

(Migrated from github.com)

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.

Another instance of excessive newlining: Before ```hs 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: ```hs 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 commented

2020-10-03 20:26:20 +02:00

(Migrated from github.com)

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.

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. --- ``` hs -- 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? --- ``` hs -- 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.) --- ``` hs -- 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! --- ``` hs -- 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. --- ``` hs -- 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. --- ``` hs -- 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. --- ``` hs -- 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 commented

2020-10-23 22:22:34 +02:00

(Migrated from github.com)

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.

> 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 commented

2020-10-23 22:51:41 +02:00

(Migrated from github.com)

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

hasufell commented

2020-10-23 23:14:24 +02:00

(Migrated from github.com)

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

Yes and it's neither configurable, nor pleasant.

> Have you tried Ormolu? https://github.com/tweag/ormolu Yes and it's neither configurable, nor pleasant.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#316