hexagoxel
/
brittany
1
0
Fork 0
Code Issues 117 Pull Requests 5 Packages Projects Releases 5 Wiki Activity

Data declaration for newtype and records #259

Merged
eborden merged 24 commits from datadecl into master 2019-12-09 22:46:04 +01:00
Conversation 39 Commits 24 Files Changed 25
eborden commented 2019-09-09 00:43:48 +02:00 (Migrated from github.com)

My intent with this PR is to take stock of the status of the datadecl branch and to push for an in progress merge. This work has been pending with sporadic changes for a long time. I believe that is because of the monolithic nature of the work. An all or none strategy has not worked and is counter to brittany's basic design on top of exact print (to be able to incrementally support new features). As well most incremental progress has been put towards refreshing the branch with each sporadic effort. This bit-rotting has been a time suck.

Instead I would like to see the current work (pending a few updates) merged with the goal of providing layout for newtype and record style data types.

Current Status

Supported

  • single records
  • multi-field records types
  • tabular alignment
  • basic deriving
  • deriving strategies
  • existential quantification

Blockers

  • retaining comments
  • CPP for older ghc versions

Deferred

  • normal types
  • sum types
My intent with this PR is to take stock of the status of the `datadecl` branch and to push for an in progress merge. This work has been pending with sporadic changes for a long time. I believe that is because of the monolithic nature of the work. An all or none strategy has not worked and is counter to `brittany`'s basic design on top of exact print (to be able to incrementally support new features). As well most incremental progress has been put towards refreshing the branch with each sporadic effort. This bit-rotting has been a time suck. Instead I would like to see the current work (pending a few updates) merged with the goal of providing layout for `newtype` and record style `data` types. ### Current Status #### Supported - single records - multi-field records types - tabular alignment - basic deriving - deriving strategies - existential quantification #### Blockers - retaining comments - `CPP` for older `ghc` versions #### Deferred - normal types - sum types
❤️ 3
lspitzner (Migrated from github.com) reviewed 2019-09-09 00:43:48 +02:00
tfausak commented 2019-09-09 14:20:49 +02:00 (Migrated from github.com)

👀 I'll review this soon. I glanced over the test cases and liked what I saw!

:eyes: I'll review this soon. I glanced over the test cases and liked what I saw!
tfausak (Migrated from github.com) reviewed 2019-09-10 01:19:33 +02:00
tfausak (Migrated from github.com) left a comment

I started reading layoutDataDecl but I can't really grok it. No fault of yours though! I'm not familiar enough with this codebase to do a thorough code review. Is there anything in particular you'd like me to look at?

I started reading `layoutDataDecl` but I can't really grok it. No fault of yours though! I'm not familiar enough with this codebase to do a thorough code review. Is there anything in particular you'd like me to look at?
src/Language/Haskell/Brittany/Internal/Types.hs
@ -185,6 +185,8 @@ data ColSig
| ColBindStmt
| ColDoLet -- the non-indented variant
| ColRec
| ColRecUpdate -- used for both RecCon and RecUpd. TODO: refactor to reflect?
tfausak (Migrated from github.com) commented 2019-09-10 00:58:58 +02:00

What would be the benefit of differentiating between record construction and record update? I thought they were always formatted the same.

What would be the benefit of differentiating between record construction and record update? I thought they were always formatted the same.
lspitzner commented 2019-10-11 01:16:43 +02:00 (Migrated from github.com)

I fully approve of the idea of getting this merged. I started working on supporting the older ghcs, and with this commit it compiles with both ghc-8.4 and ghc-8.6. Unfortunately, the tests on 8.4 do not pass.

One failure case is some whitespace problem that I have not investigated but that is probably relatively easy to fix.

The more annoying issue is that 8.4 does not have DerivingVia, which means that we need test-cases that only apply starting at some compiler version. Expanding the test file schema like this:

#group mytestgroup

#test mytestname
#pending myreason
func = ...

#test othertestname
func = ...

#test conditionaltestname
#starting-at-ghc 8.6
func = ...

should do the trick.

(this might point towards the question if we want to reduce the number of supported compiler versions, but I think I want this feature regardless; it seems it would be required again in the future even if we only supported two versions at a time.)

I plan to keep working on this when I find the time, in small pieces. If anyone else works on this over more than an evening, you might want to leave a note so that we don't conflict / do redundant work. This advice applies to myself, too.

I fully approve of the idea of getting this merged. I started working on supporting the older ghcs, and with this commit it _compiles_ with both ghc-8.4 and ghc-8.6. Unfortunately, the tests on 8.4 do not pass. One failure case is some whitespace problem that I have not investigated but that is probably relatively easy to fix. The more annoying issue is that 8.4 does not have `DerivingVia`, which means that we need test-cases that only apply starting at some compiler version. Expanding the test file schema like this: ~~~~ #group mytestgroup #test mytestname #pending myreason func = ... #test othertestname func = ... #test conditionaltestname #starting-at-ghc 8.6 func = ... ~~~~ should do the trick. (this might point towards the question if we want to reduce the number of supported compiler versions, but I think I want this feature regardless; it seems it would be required again in the future even if we only supported two versions at a time.) I plan to keep working on this when I find the time, in small pieces. If anyone else works on this over more than an evening, you might want to leave a note so that we don't conflict / do redundant work. This advice applies to myself, too.
👍 1
eborden commented 2019-10-14 16:23:11 +02:00 (Migrated from github.com)

@lspitzner The big blocker in my mind right now is comments. They are something that could precipitate a lot of change in this code (though maybe they won't 🤷‍♂️) If you have the time I might recommend doing some research on what that might look like instead of attacking multi version support.

100% agree on committing early and often and keeping the lines of communication going so we don't have redundant work.

@lspitzner The big blocker in my mind right now is comments. They are something that could precipitate a lot of change in this code (though maybe they won't :man_shrugging:) If you have the time I might recommend doing some research on what that might look like instead of attacking multi version support. 100% agree on committing early and often and keeping the lines of communication going so we don't have redundant work.
lspitzner commented 2019-10-22 11:48:12 +02:00 (Migrated from github.com)

I have started on supporting comments, will push something this night.

I have started on supporting comments, will push something this night.
👍 1
eborden commented 2019-10-22 22:31:17 +02:00 (Migrated from github.com)

I did a bit of fiddling with it, but didn't get anywhere. I'd love to have a brain dump from you on how brittany is currently handling comments.

I did a bit of fiddling with it, but didn't get anywhere. I'd love to have a brain dump from you on how `brittany` is currently handling comments.
eborden (Migrated from github.com) reviewed 2019-10-23 23:21:08 +02:00
src-literatetests/10-tests.blt
eborden (Migrated from github.com) commented 2019-10-23 23:21:07 +02:00

Yeah, that is a lot of fun!

Maybe something like this:

  -- a
  deriving --b
           ( -- c
             ToJSON -- d
           , -- e
             FromJSON --f
           ) -- g
           via  -- h
           ( -- i
             SomeType --j
           , -- k
             ABC --l
           )

The alignment with deriving would then be dropped in align left and replaced with a single indent.

  -- a
  deriving --b
    ( -- c
      ToJSON -- d
    , -- e
      FromJSON --f
    ) -- g
    via  -- h
    ( -- i
      SomeType --j
    , -- k
      ABC --l
    )
Yeah, that is a lot of fun! Maybe something like this: ```hs -- a deriving --b ( -- c ToJSON -- d , -- e FromJSON --f ) -- g via -- h ( -- i SomeType --j , -- k ABC --l ) ``` The alignment with `deriving` would then be dropped in _align left_ and replaced with a single indent. ```hs -- a deriving --b ( -- c ToJSON -- d , -- e FromJSON --f ) -- g via -- h ( -- i SomeType --j , -- k ABC --l ) ```
eborden commented 2019-11-04 04:08:11 +01:00 (Migrated from github.com)

@lspitzner I've updated the branch to build with all supported GHC versions. There are a number of test failures that will need to be picked off. One specific issue is literate tests that use syntax that is not available in earlier versions, like DerivingVia in 8.2.2.

  src-literatetests/Main.hs:151: 
  1) data type declarations record deriving via
       expected: Right 
                 data Foo = Bar
                   { foo  :: Baz
                   , bars :: Bizzz
                   }
                   deriving ToJSON via (SomeType)
                   deriving (ToJSON, FromJSON) via (SomeType)

        but got: Left "parsing error: parse error on input \8216via\8217"

We'll likely need a mechanism to disable certain tests depending on the GHC version.

@lspitzner I've updated the branch to build with all supported GHC versions. There are a number of test failures that will need to be picked off. One specific issue is literate tests that use syntax that is not available in earlier versions, like `DerivingVia` in 8.2.2. ``` src-literatetests/Main.hs:151: 1) data type declarations record deriving via expected: Right data Foo = Bar { foo :: Baz , bars :: Bizzz } deriving ToJSON via (SomeType) deriving (ToJSON, FromJSON) via (SomeType) but got: Left "parsing error: parse error on input \8216via\8217" ``` We'll likely need a mechanism to disable certain tests depending on the GHC version.
lspitzner commented 2019-11-04 17:40:50 +01:00 (Migrated from github.com)

great work, I'll have a look later. I think one set of errors comes down to one trivial mistake that causes duplication of offsets/comments in front of a data-def. That, together with the deriving-via fix you mentioned, I think 8.4 is clean already.

great work, I'll have a look later. I think one set of errors comes down to one trivial mistake that causes duplication of offsets/comments in front of a data-def. That, together with the deriving-via fix you mentioned, I think 8.4 is clean already.
lspitzner commented 2019-11-07 16:09:18 +01:00 (Migrated from github.com)

All green! nice. I still need to review the last few commits, and I want to test this on one or two larger codebases.

All green! nice. I still need to review the last few commits, and I want to test this on one or two larger codebases.
eborden commented 2019-11-07 17:14:08 +01:00 (Migrated from github.com)

Nice work on the min-ghc constraint in tests! That will come in handy. I'll give this a run on Freckle's codebase (180 KLOC) and see if anything funky pops up.

Nice work on the `min-ghc` constraint in tests! That will come in handy. I'll give this a run on Freckle's codebase (180 KLOC) and see if anything funky pops up.
👍 1
eborden commented 2019-11-07 17:46:56 +01:00 (Migrated from github.com)

I'm only seeing one issue. Interstitial comments are getting de-indented one level too much.

data CurrentLevel = CurrentLevel
   { clLevel :: FreckleTextLevel
-  -- ^ Either the last Snapshot or Starting/Overridden level
+-- ^ Either the last Snapshot or Starting/Overridden level
   , clStartingLevel :: FreckleTextLevel
-  -- ^ Specifically the Starting/Overridden level
+-- ^ Specifically the Starting/Overridden level
   , clOverridden :: Bool
-  -- ^ True if level has been Overridden
+-- ^ True if level has been Overridden
   }
   deriving Show
I'm only seeing one issue. Interstitial comments are getting de-indented one level too much. ```diff data CurrentLevel = CurrentLevel { clLevel :: FreckleTextLevel - -- ^ Either the last Snapshot or Starting/Overridden level +-- ^ Either the last Snapshot or Starting/Overridden level , clStartingLevel :: FreckleTextLevel - -- ^ Specifically the Starting/Overridden level +-- ^ Specifically the Starting/Overridden level , clOverridden :: Bool - -- ^ True if level has been Overridden +-- ^ True if level has been Overridden } deriving Show ```
👍 1
tfausak commented 2019-11-07 20:22:19 +01:00 (Migrated from github.com)

I ran this branch over ITProTV's medium sized (72 KLOC) simple codebase and only ran into one bona fide problem:

data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [ EnterpriseGrantResponse
]

(We have conf_layout.lconfig_cols set to 100.) I'm not exactly sure how that should be formatted, but I'd be alright with one of these:

data EnterpriseGrantsForCompanyResponse
  = EnterpriseGrantsForCompanyResponse Types.Company [EnterpriseGrantResponse]

data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse
  Types.Company
  [EnterpriseGrantResponse]

data EnterpriseGrantsForCompanyResponse
  = EnterpriseGrantsForCompanyResponse
    Types.Company
    [EnterpriseGrantResponse]
I ran this branch over ITProTV's medium sized (72 KLOC) simple codebase and only ran into one bona fide problem: ``` hs data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [ EnterpriseGrantResponse ] ``` (We have `conf_layout.lconfig_cols` set to `100`.) I'm not exactly sure how that should be formatted, but I'd be alright with one of these: ``` hs data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [EnterpriseGrantResponse] data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [EnterpriseGrantResponse] data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [EnterpriseGrantResponse] ```
eborden commented 2019-11-07 21:10:51 +01:00 (Migrated from github.com)

This would be consistent with how brittany formats types in signatures.

data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse
  Types.Company
  [EnterpriseGrantResponse]
This would be consistent with how `brittany` formats types in signatures. ```hs data EnterpriseGrantsForCompanyResponse = EnterpriseGrantsForCompanyResponse Types.Company [EnterpriseGrantResponse] ```
eborden commented 2019-11-07 21:56:26 +01:00 (Migrated from github.com)

@tfausak I added some tests and layouting for that situation.

@tfausak I added some tests and layouting for that situation.
eborden commented 2019-11-07 21:56:50 +01:00 (Migrated from github.com)

@lspitzner I'll need your help with the comment issue.

@lspitzner I'll need your help with the comment issue.
tfausak commented 2019-11-07 23:14:05 +01:00 (Migrated from github.com)

Awesome, thanks for the quick fix! I re-ran Brittany at 393bdb0a172bec3704d2c3ba4ff25f754ba137dc against my codebase and everything worked. I'm also seeing the comment problem, but everything builds properly after formatting.

Awesome, thanks for the quick fix! I re-ran Brittany at 393bdb0a172bec3704d2c3ba4ff25f754ba137dc against my codebase and everything worked. I'm also seeing the comment problem, but everything builds properly after formatting.
lspitzner commented 2019-11-08 00:22:02 +01:00 (Migrated from github.com)

My notes from testing on some non-public code:

  • ran into the same comment indentation problem. It is way more than "reduce by one 'layer'/tab". Need to reverse-engineer ghc-exactprint behaviour :D
  • for small records (think Point { x :: Int, y :: Int }) a one-line format might be nice;
  • for dictionary-style records (that contain functions), long type signatures are line-broken and indented not very well. This is something where I was not even sure what I had expected as ideal output;
  • for data-types with non-trivial existentials + constraints you get really nasty linebreaks;
  • this code-base puts deriving directly after the closing brace (of a record). This saves a line, but would not work well for more than one constructor. But then it is really rare that you have records + multiple constructors. It also leads to more indentation/inconsistency if you have multiple deriving (strategy) lines. Maybe I'll put this behind a config flag. (I know some of you are not a fan of more config flags. I'll ask the other people on that codebase first anyway.)

Will add test-cases/examples for the above later.

I plan to write some documentation to explain the processing of comments. I'll also look into this particular comment problem; the ghc-exactprint semantics of its DP (DeltaPosition) value are not intuitive, and this looks like another unexpected special case.

My notes from testing on some non-public code: - ran into the same comment indentation problem. It is way more than "reduce by one 'layer'/tab". Need to reverse-engineer `ghc-exactprint` behaviour :D - for small records (think `Point { x :: Int, y :: Int }`) a one-line format might be nice; - for dictionary-style records (that contain functions), long type signatures are line-broken and indented not very well. This is something where I was not even sure what I had expected as ideal output; - for data-types with non-trivial existentials + constraints you get really nasty linebreaks; - this code-base puts `deriving` directly after the closing brace (of a record). This saves a line, but would not work well for more than one constructor. But then it is really rare that you have records + multiple constructors. It also leads to more indentation/inconsistency if you have multiple deriving (strategy) lines. _Maybe_ I'll put this behind a config flag. (I know some of you are not a fan of more config flags. I'll ask the other people on that codebase first anyway.) Will add test-cases/examples for the above later. I plan to write some documentation to explain the processing of comments. I'll also look into this particular comment problem; the `ghc-exactprint` semantics of its `DP` (DeltaPosition) value are not intuitive, and this looks like another unexpected special case.
tfausak commented 2019-11-12 16:05:52 +01:00 (Migrated from github.com)

I re-ran 09da6d4ad14b9be5ec49adc05ad77bd7cd6cffac against ITProTV's code base and everything looks great!

Most of our record data declarations were written with deriving immediately after the closing brace, but I think it makes sense to put that on its own line. I'd be alright with Brittany simply choosing to put them on their own line and not having a config for it.

I re-ran 09da6d4ad14b9be5ec49adc05ad77bd7cd6cffac against ITProTV's code base and everything looks great! Most of our record data declarations were written with `deriving` immediately after the closing brace, but I think it makes sense to put that on its own line. I'd be alright with Brittany simply choosing to put them on their own line and not having a config for it.
eborden commented 2019-11-12 16:37:31 +01:00 (Migrated from github.com)

I'd be alright with Brittany simply choosing to put them on their own line and not having a config for it.

The choice to put them on a new line sacrifices one vertical line for consistency in the current GHC world where DerivingStrategies is becoming more popular. I'm in favor of keeping the simplicity of always having a new line.

> I'd be alright with Brittany simply choosing to put them on their own line and not having a config for it. The choice to put them on a new line sacrifices one vertical line for consistency in the current GHC world where `DerivingStrategies` is becoming more popular. I'm in favor of keeping the simplicity of always having a new line.
tfausak commented 2019-11-12 22:38:02 +01:00 (Migrated from github.com)

Ah, I found a problem. Trying to format a data declaration like this:

data T = C {}

Returns a syntactically invalid result:

data T =

I think it should probably keep the empty braces around, but as far as I know that's the same as not taking any arguments. I'd be fine with either of these:

data T = C
data T = C {}
Ah, I found a problem. Trying to format a data declaration like this: ``` hs data T = C {} ``` Returns a syntactically invalid result: ``` hs data T = ``` I think it should probably keep the empty braces around, but as far as I know that's the same as not taking any arguments. I'd be fine with either of these: ``` hs data T = C data T = C {} ```
eborden commented 2019-11-12 23:03:05 +01:00 (Migrated from github.com)

@tfausak I've added a test and corrected that bug.

@tfausak I've added a test and corrected that bug.
tfausak commented 2019-11-14 00:04:55 +01:00 (Migrated from github.com)

Awesome, thanks!

Awesome, thanks!
eborden commented 2019-11-14 16:57:45 +01:00 (Migrated from github.com)

@lspitzner do you have an examples of these two?

  • for dictionary-style records (that contain functions), long type signatures are line-broken and indented not very well. This is something where I was not even sure what I had expected as ideal output;
  • for data-types with non-trivial existentials + constraints you get really nasty linebreaks;
@lspitzner do you have an examples of these two? > - for dictionary-style records (that contain functions), long type signatures are line-broken and indented not very well. This is something where I was not even sure what I had expected as ideal output; > - for data-types with non-trivial existentials + constraints you get really nasty linebreaks;
lspitzner commented 2019-11-18 11:29:48 +01:00 (Migrated from github.com)

I have written a doc chapter on comment (and DeltaPosition) handling: https://github.com/lspitzner/brittany/blob/master/doc/implementation/exactprinting.md

I have written a doc chapter on comment (and DeltaPosition) handling: https://github.com/lspitzner/brittany/blob/master/doc/implementation/exactprinting.md
lspitzner commented 2019-11-18 11:38:50 +01:00 (Migrated from github.com)

@eborden

data Single = Single { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong }

data Double = Double { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong, bar :: Int }

data Constrained = forall a b . (Loooooooooooooooooooooooooooooooong a, Loooooooooooooooooooooooooooooooong b) => Constrained { a :: a, b :: b}
@eborden ~~~~.hs data Single = Single { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong } data Double = Double { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong, bar :: Int } data Constrained = forall a b . (Loooooooooooooooooooooooooooooooong a, Loooooooooooooooooooooooooooooooong b) => Constrained { a :: a, b :: b} ~~~~
lspitzner commented 2019-11-18 11:42:07 +01:00 (Migrated from github.com)

"Good" layout might be something like

data Single = Single
  { foo
      :: loooooooooooooooooooooooooooooooong
      -> loooooooooooooooooooooooooooooooong
  }

data Double = Double
  { foo
      :: loooooooooooooooooooooooooooooooong
      -> loooooooooooooooooooooooooooooooong
  , bar :: Int
  }

data Constrained =
  forall a b .
  ( Loooooooooooooooooooooooooooooooong a
  , Loooooooooooooooooooooooooooooooong b
  ) => Constrained
    { a :: a, b :: b }

although I am really not sure about the last one.

"Good" layout might be something like ~~~~.hs data Single = Single { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong } data Double = Double { foo :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong , bar :: Int } data Constrained = forall a b . ( Loooooooooooooooooooooooooooooooong a , Loooooooooooooooooooooooooooooooong b ) => Constrained { a :: a, b :: b } ~~~~ although I am really not sure about the last one.
tfausak commented 2019-11-18 13:45:06 +01:00 (Migrated from github.com)

Those looks reasonable to me. I feel like forall always throws a wrench into layouting.

What about the case where two fields have the same long function type?

data Weird = Weird { foo, bar :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong }
Those looks reasonable to me. I feel like `forall` always throws a wrench into layouting. What about the case where two fields have the same long function type? ``` hs data Weird = Weird { foo, bar :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong } ```
lspitzner commented 2019-11-19 10:56:28 +01:00 (Migrated from github.com) 
data Weird = Weird
  { foo, bar
      :: loooooooooooooooooooooooooooooooong
      -> loooooooooooooooooooooooooooooooong
  }

I guess? I would frankly ignore the case where the comma-separated names overflow.

~~~~.hs data Weird = Weird { foo, bar :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong } ~~~~ I guess? I would frankly ignore the case where the comma-separated names overflow.
👍 2
lspitzner commented 2019-11-23 15:40:44 +01:00 (Migrated from github.com)

I am working on the cases mentioned in https://github.com/lspitzner/brittany/pull/259#issuecomment-554959278.

I am working on the cases mentioned in https://github.com/lspitzner/brittany/pull/259#issuecomment-554959278.
👍 1
lspitzner commented 2019-11-25 23:51:01 +01:00 (Migrated from github.com)

The tests I came up with all pass again. Found and fixed some more bugs around cases where things don't fit in one line.

Some notes about the last commit that go beyond what we already mentioned:

  1. I disabled the single-line layout by default, but invented a new config flag to allow single-line layout for any number of fields (instead of exactly one previously). Now that I think about this though, I am inclined to comment that flag out again until someone asks for it. I don't think that will happen - single field data (newtype is different!) is rare, and fields on multiple lines is good for readability even for small structs.

  2. I opted to go with the "prefix" style over the end-of-line style, i.e.

    data Foo
  = forall e
  . SomeLongConstraint e => Bar

    over

    data Foo =
  forall e .
  SomeLongConstraint e => Bar

    to increase consistency with multi-constructor data decls that we will support in the future; for those I think we'll use

    data Either a b
  = Left a
  | Right b

  3. For the RHS constraint I did however not put "=>" at the start of the line. Makes the indentation amount odd. A somewhat full example is

    data MyRecord
  = forall a b
  . ( Loooooooooooooooooooooooooooooooong a
    , Loooooooooooooooooooooooooooooooong b
    ) =>
    MyConstructor
      { foo, foo2
          :: loooooooooooooooooooooooooooooooong
          -> loooooooooooooooooooooooooooooooong
      , bar  :: a
      , bazz :: b
      }
  deriving Show

oh, and now I notice that if your derivings can overflow. That's annoying, although it probably is
rare for data. For newtype there definitely are cases with more than one line of generalized-newtype-deriving instances. E.g.

newtype UserId = UserId { getUserId :: Int }
  deriving (Show, Read, Eq, Ord, Enum, Integral, Generic, Typeable, Num, ToJSON, FromJSON, ToField, ToRow)

For newtypes I really would like some fancy unique+paragraph-fill solution, so you get

newtype UserId = UserId { getUserId :: Int }
  deriving (Show, Read, Eq, Ord, Enum, Integral, Generic, Typeable)
  deriving (Num, ToJSON, FromJSON, ToField, ToRow)

Let's leave this for the newtype layouting implementation though.

The tests I came up with all pass again. Found and fixed some more bugs around cases where things don't fit in one line. Some notes about the last commit that go beyond what we already mentioned: 1. I disabled the single-line layout by default, but invented a new config flag to allow single-line layout for any number of fields (instead of exactly one previously). Now that I think about this though, I am inclined to comment that flag out again until someone asks for it. I don't think that will happen - single field `data` (`newtype` is different!) is rare, and fields on multiple lines is good for readability even for small structs. 1. I opted to go with the "prefix" style over the end-of-line style, i.e. ~~~~.hs data Foo = forall e . SomeLongConstraint e => Bar ~~~~ over ~~~~.hs data Foo = forall e . SomeLongConstraint e => Bar ~~~~ to increase consistency with multi-constructor data decls that we will support in the future; for those I think we'll use ~~~~.hs data Either a b = Left a | Right b ~~~~ 2. For the RHS constraint I did however _not_ put "=>" at the start of the line. Makes the indentation amount odd. A somewhat full example is ~~~~.hs data MyRecord = forall a b . ( Loooooooooooooooooooooooooooooooong a , Loooooooooooooooooooooooooooooooong b ) => MyConstructor { foo, foo2 :: loooooooooooooooooooooooooooooooong -> loooooooooooooooooooooooooooooooong , bar :: a , bazz :: b } deriving Show ~~~~ ---- oh, and now I notice that if your derivings can overflow. That's annoying, although it probably is rare for `data`. For `newtype` there definitely are cases with more than one line of generalized-newtype-deriving instances. E.g. ~~~~.hs newtype UserId = UserId { getUserId :: Int } deriving (Show, Read, Eq, Ord, Enum, Integral, Generic, Typeable, Num, ToJSON, FromJSON, ToField, ToRow) ~~~~ For newtypes I really would like some fancy unique+paragraph-fill solution, so you get ~~~~.hs newtype UserId = UserId { getUserId :: Int } deriving (Show, Read, Eq, Ord, Enum, Integral, Generic, Typeable) deriving (Num, ToJSON, FromJSON, ToField, ToRow) ~~~~ Let's leave this for the `newtype` layouting implementation though.
eborden commented 2019-11-26 00:06:27 +01:00 (Migrated from github.com)

@lspitzner I have a separate branch based off this to support sum types. When this is ready I can merge the differences in. Are we ready to give this a 👍 and merge?

@lspitzner I have a separate branch based off this to support sum types. When this is ready I can merge the differences in. Are we ready to give this a :+1: and merge?
lspitzner commented 2019-11-27 21:42:40 +01:00 (Migrated from github.com)

I have tested it on what I have available and this looks good to merge. @tfausak could you perhaps check this one more time on your larger codebase(s) please?

I have tested it on what I have available and this looks good to merge. @tfausak could you perhaps check this one more time on your larger codebase(s) please?
eborden commented 2019-12-09 19:15:39 +01:00 (Migrated from github.com)

@lspitzner Any conclusions on merging this? This PR is already bit-rotting and conflicts are sneaking in. I'd love to clean up these merge conflicts and merge this current batch of work. Improvements can be deferred to further PRs.

@lspitzner Any conclusions on merging this? This PR is already bit-rotting and conflicts are sneaking in. I'd love to clean up these merge conflicts and merge this current batch of work. Improvements can be deferred to further PRs.
👍 1
eborden commented 2019-12-09 19:23:33 +01:00 (Migrated from github.com)

☝️ pushed up conflict fixes. Please git pull -r if you have additional commits to add.

:point_up: pushed up conflict fixes. Please `git pull -r` if you have additional commits to add.
tfausak commented 2019-12-09 19:53:26 +01:00 (Migrated from github.com)

@tfausak could you perhaps check this one more time on your larger codebase(s) please?

Sorry, I didn't see this until now. I re-ran the latest commit (a1282c3ac6) against the ITProTV code base and everything looked great! 👍

> @tfausak could you perhaps check this one more time on your larger codebase(s) please? Sorry, I didn't see this until now. I re-ran the latest commit (a1282c3ac670b61e6bbcd50cf7a12612b385b2e5) against the ITProTV code base and everything looked great! :+1:
lspitzner commented 2019-12-09 20:19:13 +01:00 (Migrated from github.com)

@tfausak great, thanks.

@eborden thanks for the rebase. I have found one glitch in a special case that I have not had time to look at in the last few days. Will have a look now. If it has a simple fix I'd like to include it, otherwise I'll merge this and release. Will hopefully get back to you in the next hour or so.

@tfausak great, thanks. @eborden thanks for the rebase. I have found one glitch in a special case that I have not had time to look at in the last few days. Will have a look now. If it has a simple fix I'd like to include it, otherwise I'll merge this and release. Will hopefully get back to you in the next hour or so.
👍 1
eborden commented 2019-12-09 20:28:57 +01:00 (Migrated from github.com)

@lspitzner I'm not necessarily pushing for a release. I'm just pushing for merging to master. If there are further improvements to be made before a release then we should defer releasing until they are also merged to master.

@lspitzner I'm not necessarily pushing for a release. I'm just pushing for merging to `master`. If there are further improvements to be made before a release then we should defer releasing until they are also merged to `master`.
lspitzner commented 2019-12-09 22:39:34 +01:00 (Migrated from github.com)

But once it is on master, we cannot make releases without this feature. And I'd like to make a new release with the regression fixes that are on master asap.

I'll just make a patch release 0.12.1.1 and merge this immediately after. I am not happy with my latest commit (had to add another Bool to the BriDoc datatype..) but it passes all tests now. I am pretty certain that this only affects rather special cases.

But once it is on master, we cannot make releases without this feature. And I'd like to make a new release with the regression fixes that are on master asap. I'll just make a patch release 0.12.1.1 and merge this immediately after. I am not happy with my latest commit (had to add another Bool to the BriDoc datatype..) but it passes all tests now. I am pretty certain that this only affects rather special cases.
lspitzner commented 2019-12-09 22:58:30 +01:00 (Migrated from github.com)

Thanks again @eborden . I have just released 0.12.1.1 without this feature, but we will definitely include it in the next release. I really think this is ready for publication even when it does not cover mulitple constructors yet; I just wanted to get the bugfixes out without having to organize the announcement that we should have with the new feature release.

Thanks again @eborden . I have just released 0.12.1.1 _without_ this feature, but we will definitely include it in the next release. I really think this is ready for publication even when it does not cover mulitple constructors yet; I just wanted to get the bugfixes out without having to organize the announcement that we should have with the new feature release.
eborden commented 2019-12-10 04:00:30 +01:00 (Migrated from github.com)

Thanks @lspitzner. My current plan is working on sums next, which I'll follow with GADTs. Let me know if this conflicts with any of your initiatives.

Thanks @lspitzner. My current plan is working on sums next, which I'll follow with GADTs. Let me know if this conflicts with any of your initiatives.
Sign in to join this conversation.
Reviewers
No reviewers
lspitzner
Labels
Clear labels   
blocked: dependency
   
blocked: info-needed
   
bug
   
duplicate
   
enhancement
   
fixed in HEAD
   
help wanted
   
hs:arrows
   
hs:brackets
   
hs:classes
   
hs:comments
   
hs:do-notation
   
hs:guards
   
hs:lists
   
hs:operators
   
hs:patterns
   
hs:records
   
hs:types
   
invalid
   
language extension support
   
layouting
   
needs confirmation
   
priority: high
   
priority: low
   
question
   
revisit before next release
   
wontfix
No Label
blocked: dependency
blocked: info-needed
bug
duplicate
enhancement
fixed in HEAD
help wanted
hs:arrows
hs:brackets
hs:classes
hs:comments
hs:do-notation
hs:guards
hs:lists
hs:operators
hs:patterns
hs:records
hs:types
invalid
language extension support
layouting
needs confirmation
priority: high
priority: low
question
revisit before next release
wontfix
Milestone
Clear milestone
No items
No Milestone
Version 1.0.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
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#259
There is no content yet.
Delete Branch "datadecl"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?