lspitzner / brittany

haskell source code formatter
GNU Affero General Public License v3.0
688 stars 72 forks source link

layout of imports uses copious amounts of horizontal&vertical space #190

Open 2mol opened 5 years ago

2mol commented 5 years ago

Hi, I have a file with the following imports:

module Lib where

import Config (Config)
import qualified Config
import Control.Exception as E
import qualified Data.Char as C
import qualified Data.Either.Combinators as Either
import Data.Function ((&))
import qualified Data.List as List
import Data.List.NonEmpty (NonEmpty (..))
import qualified Data.Maybe as Maybe
import Data.Text (Text)
import qualified Data.Text as T
import Data.Text.Titlecase (titlecase)
import Data.Time.Clock (UTCTime)
import GHC.Exts (sortWith)
import Lens.Micro ((^.))
import Path (Abs, Dir, File, Path, (</>))
import qualified Path
import qualified Path.IO as Path
import qualified System.FilePath as F
import qualified System.Process as P
import qualified Text.PDF.Info as PDFI

As you can see if you try formatting it for example with https://hexagoxel.de/brittany/, the output will be very ugly. I see 2 problems:

  1. The listed (the things in the round brackets) imports are put on separate lines.
  2. There is unnecessary space introduced after the longest imported module name.

I'll paste the output as well as the result of stylish-haskell for reference:

Brittany:

module Lib where

import           Config                         ( Config )
import qualified Config
import           Control.Exception             as E
import qualified Data.Char                     as C
import qualified Data.Either.Combinators       as Either
import           Data.Function                  ( (&) )
import qualified Data.List                     as List
import           Data.List.NonEmpty             ( NonEmpty(..) )
import qualified Data.Maybe                    as Maybe
import           Data.Text                      ( Text )
import qualified Data.Text                     as T
import           Data.Text.Titlecase            ( titlecase )
import           Data.Time.Clock                ( UTCTime )
import           GHC.Exts                       ( sortWith )
import           Lens.Micro                     ( (^.) )
import           Path                           ( Abs
                                                , Dir
                                                , File
                                                , Path
                                                , (</>)
                                                )
import qualified Path
import qualified Path.IO                       as Path
import qualified System.FilePath               as F
import qualified System.Process                as P
import qualified Text.PDF.Info                 as PDFI

stylish-haskell:

module Lib where

import           Config                  (Config)
import qualified Config
import           Control.Exception       as E
import qualified Data.Char               as C
import qualified Data.Either.Combinators as Either
import           Data.Function           ((&))
import qualified Data.List               as List
import           Data.List.NonEmpty      (NonEmpty (..))
import qualified Data.Maybe              as Maybe
import           Data.Text               (Text)
import qualified Data.Text               as T
import           Data.Text.Titlecase     (titlecase)
import           Data.Time.Clock         (UTCTime)
import           GHC.Exts                (sortWith)
import           Lens.Micro              ((^.))
import           Path                    (Abs, Dir, File, Path, (</>))
import qualified Path
import qualified Path.IO                 as Path
import qualified System.FilePath         as F
import qualified System.Process          as P
import qualified Text.PDF.Info           as PDFI
lspitzner commented 5 years ago

I have added a section to the brittany rationale that explains the current behaviour, and highlights the trade-off of this decision. The "context-sensitive" term is introduced above.

As mentioned there, this decision is not final, and I am open to merging a PR that adds paragraph-fill behaviour for those that want this. Even changing the default is on the table.

ElvishJerricco commented 5 years ago

I'd like an config option for avoiding all the excess whitespace altogether as well. i.e. have brittany produce pretty much exactly what @2mol used as the original input, plus paragraph-fill behavior.

2mol commented 5 years ago

Thanks for the detailed rationale/writeup! It now makes a lot more sense and I agree with a lot of your points, especially the context insensitivity and the merge/blame friendliness.

Where I slightly disagree is the notion of not looking at imports a lot. In my case I have the opposite experience, having readable imports is something I need a lot because I'm constantly figuring out where 20 function names and 12 operators are coming from. But maybe I lack Haskell experience.

Some points:

  1. what would happen if (in paragraph-fill) a module name would be so long that it intrudes on your static column?
  2. Having an option for least amount of whitespace (my first input) would be a great compromise.
  3. I quite like how elm-format does it, have had a look at that? It's pretty much what @ElvishJerricco suggested, sorted alphabetically. It's nicer for them though, since they don't have the explicit 'qualified' keyword amongst other things.
eborden commented 5 years ago

I'm also very much in support of a "compact" form of import layout. Freckle currently uses stylish-haskell with the following config:

  - imports:
      align: none
      list_align: after_alias
      pad_module_names: false
      long_list_align: new_line_multiline
      empty_list_align: right_after
      list_padding: 2
      separate_lists: false
      space_surround: false

To get imports like:

import Database.Esqueleto hiding (Value)
import qualified Database.Persist as P
import FrontRow.Entities.School
import FrontRow.Entities.Teacher
import FrontRow.Import.Yesod hiding (on, (==.))
import Text.Printf (printf)
tfausak commented 5 years ago

I think IndentPolicyLeft solves things here? Running this through Brittany does not introduce any extra spacing:

-- brittany { lconfig_indentPolicy: IndentPolicyLeft }
import Config (Config)
import qualified Config
import Control.Exception as E
import qualified Data.Char as C
import qualified Data.Either.Combinators as Either
import Data.Function ((&))
import qualified Data.List as List
import Data.List.NonEmpty (NonEmpty(..))
import qualified Data.Maybe as Maybe
import Data.Text (Text)
import qualified Data.Text as T
import Data.Text.Titlecase (titlecase)
import Data.Time.Clock (UTCTime)
import GHC.Exts (sortWith)
import Lens.Micro ((^.))
import Path (Abs, Dir, File, Path, (</>))
import qualified Path
import qualified Path.IO as Path
import qualified System.FilePath as F
import qualified System.Process as P
import qualified Text.PDF.Info as PDFI
lpil commented 5 years ago

Thank you @tfausak ! It'd be great for this to be easier to discover, is there a suitable place to document this?

eborden commented 5 years ago

@lpil documentation of the config file is in the roadmap.

eborden commented 5 years ago

@lpil the issue tracking it :point_right: https://github.com/lspitzner/brittany/issues/145