tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

More generous vertical whitespace policy #418

Closed intractable closed 5 years ago

intractable commented 5 years ago

Thanks for the cool tool! I'm currently evaluating use of ormolu at work on production codebases.

One barrier for adoption for us is that we tend to use liberal vertical whitespace for readability purposes.

For example, prior to application of ormolu, we have some code that looks like this:

env :: Options -> Managed Env
env options@Options {logLevel, dir} = do

    -- Logger configuration

    logger <- Logger.newLogger logLevel

    let log :: MonadIO m => Logger.LogLevel -> Maybe Text -> Text -> [Aeson.Pair] -> m ()
        log level (fmap encodeUtf8 -> ctx) msg pairs = liftIO do
          Logger.loggerJSONAtLevel level logger msg mempty (Aeson.object pairs)

    -- Misc setup etc.

    let mkDir d = path <$ Turtle.mktree path where path = dir </> d

    a <- mkDir "a"
    b <- mkDir "b"
    c <- mkDir "c"

    -- Cleanup on termination

    managed_ do
      bracket_ pass do
        let purge path = Turtle.sh do Turtle.rm =<< Turtle.ls path
        purge a
        purge b

    -- Lorem ipsum dolor sit amet, consectetur adipiscing
    -- elit. Quisque facilisis tellus eu nulla dictum, sit amet
    -- commodo nibh vestibulum. Aenean lacinia quam nec pharetra
    -- pellentesque. Mauris laoreet viverra sem, in sollicitudin
    -- mi vulputate vel. Suspendisse neque quam, tincidunt sit
    -- amet efficitur ut, pharetra eget orci.

    Turtle.export "TMPDIR" =<< do format fp <$> Turtle.mktempdir tmp "scratch"

    pure Env{..}

which after formatting looks like:

env :: Options -> Managed Env
env options@Options {logLevel, dir} = do
  -- Logger configuration

  logger <- Logger.newLogger logLevel
  let log :: MonadIO m => Logger.LogLevel -> Maybe Text -> Text -> [Aeson.Pair] -> m ()
      log level (fmap encodeUtf8 -> ctx) msg pairs = liftIO do
        Logger.loggerJSONAtLevel level logger msg mempty (Aeson.object pairs)
  -- Misc setup etc.

  let mkDir d = path <$ Turtle.mktree path where path = dir </> d
  a <- mkDir "a"
  b <- mkDir "b"
  c <- mkDir "c"
  -- Cleanup on termination

  managed_ do
    bracket_ pass do
      let purge path = Turtle.sh do Turtle.rm =<< Turtle.ls path
      purge a
      purge b
  -- Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque
  -- facilisis tellus eu nulla dictum, sit amet commodo nibh
  -- vestibulum. Aenean lacinia quam nec pharetra pellentesque. Mauris laoreet
  -- viverra sem, in sollicitudin mi vulputate vel. Suspendisse neque quam,
  -- tincidunt sit amet efficitur ut, pharetra eget orci.

  Turtle.export "TMPDIR" =<< do format fp <$> Turtle.mktempdir tmp "scratch"
  pure Env {..}

I'm interested to know if there's openness to contributions for employing a vertical whitespace policy which leaves more of the original vertical formatting intact (especially around comment blocks that might vertically delimit sections of code).

E.g., a policy that would format the original code to something like this:

env :: Options -> Managed Env
env options@Options {logLevel, dir} = do
  -- Logger configuration

  logger <- Logger.newLogger logLevel
  let log :: MonadIO m => Logger.LogLevel -> Maybe Text -> Text -> [Aeson.Pair] -> m ()
      log level (fmap encodeUtf8 -> ctx) msg pairs = liftIO do
        Logger.loggerJSONAtLevel level logger msg mempty (Aeson.object pairs)

  -- Misc setup etc.

  let mkDir d = path <$ Turtle.mktree path where path = dir </> d
  a <- mkDir "a"
  b <- mkDir "b"
  c <- mkDir "c"

  -- Cleanup on termination

  managed_ do
    bracket_ pass do
      let purge path = Turtle.sh do Turtle.rm =<< Turtle.ls path
      purge a
      purge b

  -- Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque
  -- facilisis tellus eu nulla dictum, sit amet commodo nibh
  -- vestibulum. Aenean lacinia quam nec pharetra pellentesque. Mauris laoreet
  -- viverra sem, in sollicitudin mi vulputate vel. Suspendisse neque quam,
  -- tincidunt sit amet efficitur ut, pharetra eget orci.

  Turtle.export "TMPDIR" =<< do format fp <$> Turtle.mktempdir tmp "scratch"
  pure Env {..}

or perhaps even be more liberal with whitespace preservation around contiguous lines of code (e.g., retaining the vertical whitespace between let mkDir = and its first use, or logger <- ... and the following let binding).

mrkkrp commented 5 years ago

Isn't this the same as #74 which #394 tries to implement? (Although I do not see lots of activity on that one.) I think we're in favor of this, so if you implement the changes they are likely to be merged.

intractable commented 5 years ago

@mrkkrp Sorry, I had somehow missed the other issue in my cursory skim. I will close this in favor of #74 and will comment on the latter if we start to implement it or pick up where the PR left off. I mostly wanted to know if the change would be welcome before getting adoption buy-in with the rest of my team.

Thanks!

mrkkrp commented 5 years ago

@intractable You said "one barrier", what are the others?

intractable commented 5 years ago

@mrkkrp Adoption of automated source code formatting tools in general, but I'm motivated to convince, and I'm a fan of the approach this project is taking.

intractable commented 5 years ago

@mrkkrp There may also be some friction around not being able to globally apply ormolu due to usage of CPP -- https://github.com/tweag/ormolu/issues/415 might help with that considerably, if implemented.

It's fine to manually remove CPP directives / format / re-insert directives in the short- to medium- term, but eventually I'd like to have CI rejections based on the result of --mode check applied to everything without the need to maintain a module blacklist or ignore the parsing failures that come up if --tolerate-cpp is used.