Skip to content

Conversation

@maksbotan
Copy link
Contributor

This will format records like this:

module Herp where

data Foo a
  = Foo
      { a :: Int
      , a2 :: String
        -- ^ some haddock
      }
  | Bar
      { b :: a
      }
  deriving (Eq, Show)
  deriving (ToJSON) via Bar Foo

Goals of this change are:

  • Achieve uniform style for data with one and many constructors by
    always starting the first constructor on the new line and aligning =
    and | symbols;
  • Visually separate fields from constructor names;
  • Keep a column with {, ,, } clean by indenting line comments two
    spaces to the right.

Personally I find this style much more pleasant than one introduced in #256. If you don't want to have it as the default one, how about making it at least configurable?


This PR is made on top of #265 and so depends on its merge. Once you merge it after your comments and edits, I will rebase this one on top of the new master. However, I would like to open discussion about this change already now.

@maksbotan
Copy link
Contributor Author

One possible alternative is to set indent before = fixed to 3 spaces, so that constructor names are aligned with type names:

module Herp where

data Foo a
   = Foo
       { a :: Int
       , a2 :: String
       }
   | Bar
       { b :: a
       }

@kirelagin
Copy link

I am not 100% sure how it is formatted currently, but in what I see in your example I explicitly like the alignment of =, |, and deriving. And, if by your first bullet point you mean that a single constructor will record fields (and only with record fields) will not be left at the same line as the data definition, then I like it too. I also like the alignment of -- ^ with record names.

That said, I don’t really see any point in indenting the {,} line two extra spaces from the constructor names.

@kirelagin
Copy link

As to the misalignment of constructor names with the data name, that is not something that I ever notice, i.e. it does not bother me enough to justify a fixed 3-space indent, which a) is a weird number; b) takes away the flexibility of using 2/4/8 spaces or tabs. But that’s a personal opinion.

@lukasz-golebiewski
Copy link
Member

Each and every format is going to have it's proponents and opponents. I would suggest adding a flag for every customization so that everyone can adjust the formatter to suit their needs.
E.g.

records: 
  - newlineFirstConstructor: true
  - blockIndentSize: 3
  (...)

@maksbotan
Copy link
Contributor Author

Thanks for the comment, @lukasz-golebiewski!
I will try to add options for all possible indents and new lines. Hope @jaspervdj would be okay with that.

This will format records like this:

module Herp where

data Foo a
  = Foo
      { a :: Int
      , a2 :: String
        -- ^ some haddock
      }
  | Bar
      { b :: a
      }
  deriving (Eq, Show)
  deriving (ToJSON) via Bar Foo

Goals of this change are:

- Achieve uniform style for data with one and many constructors by
always starting the first constructor on the new line and aligning `=`
and `|` symbols;
- Visually separate fields from constructor names;
- Keep a column with `{`, `,`, `}` clean by indenting line comments two
spaces to the right.
@maksbotan maksbotan force-pushed the maksbotan/restyle-records branch from 48365b1 to 7f1ac29 Compare February 2, 2020 16:28
@maksbotan
Copy link
Contributor Author

I've only rebased this branch atop of new master after merge of #265. New changes will come soon.

@jaspervdj
Copy link
Member

Idea sounds great so far! Looking forward to this.

@maksbotan
Copy link
Contributor Author

So, I've introduced some options to control formatting:

data Indent
    = SameLine
    | Indent !Int
  deriving (Show)

data Config = Config
    { cEquals           :: !Indent
      -- ^ Indent between type constructor and @=@ sign (measured from column 0)
    , cFirstField       :: !Indent
      -- ^ Indent between data constructor and @{@ line (measured from column with data constructor name)
    , cFieldComment     :: !Int
      -- ^ Indent between column with @{@ and start of field line comment (this line has @cFieldComment = 2@)
    , cDeriving         :: !Int
      -- ^ Indent before @deriving@ lines (measured from column 0)
    } deriving (Show)

The style I proposed originally is achieved with these settings:

  - records:
      equals: "indent 2"
      first_field: "indent 2"
      field_comment: 2
      deriving: 2

Other sensible possibilities are:

  - records:
      equals: "same_line"
      first_field: "indent 2"
      field_comment: 2
      deriving: 2
data Foo a = Foo
               { a :: Int
               , a2 :: String
                 -- ^ some haddock
               }
           | Bar
               { b :: a
               }
  deriving (Eq, Show)
  deriving (ToJSON) via Bar Foo

  - records:
      equals: "same_line"
      first_field: "same_line"
      field_comment: 2
      deriving: 2
data Foo a = Foo { a :: Int
                 , a2 :: String
                   -- ^ some haddock
                 }
           | Bar { b :: a
                 }
  deriving (Eq, Show)
  deriving (ToJSON) via Bar Foo

  - records:
      equals: "indent 2"
      first_field: "same_line"
      field_comment: 2
      deriving: 2
data Foo a
  = Foo { a :: Int
        , a2 :: String
          -- ^ some haddock
        }
  | Bar { b :: a
        }
  deriving (Eq, Show)
  deriving (ToJSON) via Bar Foo

I chose to always align , with { and | with = and to always start deriving on the new line (because there may be many deriving lines with different strategies).

What do you think? I can make it more configurable, but personally I don't think that would make more sense.

P.S. I'm not touching tests until we agree on the implementation.

@maksbotan
Copy link
Contributor Author

@jaspervdj @lukasz-golebiewski hi! Did you have a change to look at this?

Copy link
Member

@jaspervdj jaspervdj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome work! Thanks a lot.

@maksbotan
Copy link
Contributor Author

Okay, I've updated docs and tests :)

@maksbotan
Copy link
Contributor Author

Ping?

@jaspervdj
Copy link
Member

The output/configuration looks good to me but I'd like to wait until @lukasz-golebiewski or @EncodePanda have had a chance to look at the code, since they wrote the initial version of the record formatting.

Copy link
Member

@lukasz-golebiewski lukasz-golebiewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
You need to resolve conflicts before we can merge

@maksbotan
Copy link
Contributor Author

@lukasz-golebiewski thanks!
I've merged master into my branch and resolved conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants