tweag / ormolu

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

Haddock docstring placement for constructors #583

Open aspiwack opened 4 years ago

aspiwack commented 4 years ago

Is your feature request related to a problem? Please describe.

The following snippet

data Foo
 = Bar -- ^ bar
 | Baz -- ^ baz

is formatted as

data Foo
  = -- | bar
    Bar
  | -- | baz
    Baz

It's a bit awkward: the Haddock comments are placed before the constructor, probably because this is the placement for record fields. However, the reason why they are placed first in fields is because the punctuation is a the end of the line; in constructor, though, the “punctuation” is at the beginning of the line.

Describe the solution you'd like

I think either of the two formatting would be more consistent:

mrkkrp commented 4 years ago

So far we have opted for using the pipe style -- | for everything. I personally don't see why this style seems awkward. Why can't Haddocks be placed before constructors? What aesthetic principle does it violate?

aspiwack commented 4 years ago

Well, we're talking code formatting, of course. With the obvious dose of subjectivity that it entails.

But what got me to make this suggestion was that the comment is in-betweeen the “punctuation” symbol and the constructor. And it makes the indentation of the main code look jagged to me, with Bar and Baz at indentation 4, while | and = are at indentation 2.

neongreen commented 4 years ago

I am also a bit irked by that style. At the same time I dislike seeing -- ^ for longform comments.

My preference is as follows:

data SumEncoding
    -- | A constructor will be encoded to an object with a field
    -- 'tagFieldName' which specifies the constructor tag (modified by
    -- the 'constructorTagModifier').
  = TaggedObject
      { tagFieldName :: String,
        contentsFieldName :: String
      }
    -- | Only the contents of the constructor will be encoded as if
    -- the type had a single constructor.
  | UntaggedValue
    -- | A constructor will be encoded to an object with a single
    -- field named after the constructor tag
  | ObjectWithSingleField
    -- | A constructor will be encoded to a 2-element array where the
    -- first element is the tag of the constructor.
  | TwoElemArray
  deriving (Eq, Show)

This indentation is the easiest to read for me because each "section" in the data type stands out. This will also work but is a bit worse:

data SumEncoding
  -- | A constructor will be encoded to an object with a field
  -- 'tagFieldName' which specifies the constructor tag (modified by
  -- the 'constructorTagModifier').
  = TaggedObject
      { tagFieldName :: String,
        contentsFieldName :: String
      }
  -- | Only the contents of the constructor will be encoded as if
  -- the type had a single constructor.
  | UntaggedValue
  -- | A constructor will be encoded to an object with a single
  -- field named after the constructor tag
  | ObjectWithSingleField
  -- | A constructor will be encoded to a 2-element array where the
  -- first element is the tag of the constructor.
  | TwoElemArray
  deriving (Eq, Show)
aspiwack commented 4 years ago

I didn't realise @neongreen 's suggestion was possible, to be honest. It's just as fine with me.

mrkkrp commented 4 years ago

It is not impossible, but much much harder to implement.

lehmacdj commented 2 years ago

I want to add that the haddock placement for a constructor for a single constructor with fields is quite awkward at the moment.

data FutureSumType = -- | This is what the documentation for this type looks like.
  -- It is pretty awkward that the @--@s aren't aligned.
  SingleConstructor
  { someRecordField :: Bool,
    someOtherRecordField :: Int
  }
Even this admittedly very weird edge case of a single constructor data type with record syntax but no fields is broken in the same way. (click for example) ```haskell data FutureSumType = -- | This admittedly very weird edge case also -- exhibits the same broken behavior as my original example. SingleConstructor { } ```
If there are multiple constructors or if the single constructor isn't a record, the formatting is handled with good alignment as I desire. (click for example) ```haskell data FutureSumType = -- | This is what the documentation for this type looks like. -- the formatting is a lot more consistent if there aren't record fields. SingleConstructor Bool Int ``` ```haskell data FutureSumType = -- | This is what the documentation for this type looks like. -- the formatting is a lot more consistent if there aren't record fields. SingleConstructor ``` ```haskell data FutureSumType = -- | This is what the documentation for this type looks like. -- It is pretty awkward that the @--@s aren't aligned. ConstructorOne { someRecordField :: Bool, someOtherRecordField :: Int } | ConstructorTwo ```

To preserve alignment of the --s I think my original sample should instead be formatted consistently with the non-record or non-single constructor data type examples so that it would look like:

data FutureSumType
  = -- | This is what the documentation for this type looks like.
    -- It is pretty awkward that the @--@s aren't aligned.
    SingleConstructor
      { someRecordField :: Bool,
        someOtherRecordField :: Int
      }

Obviously if a different format for constructor haddocks is chosen, the same principal can be used: format single constructor haddocks the same way as haddocks in other cases.

I like this for a few reasons:

mrkkrp commented 2 years ago

@lehmacdj Good point, but I think this deserves its own issue.