Disable 1-field simplification by default #1321
Conversation
This builds on top of #1315 to minimize disruption by disabling the breaking change by default and instead requiring the user to opt in by setting a new `collapseSingletonRecords` option to `True`. The additional tests added to verify this also caught a bug in the `Interpret` instance for functions, which this change also fixes.
There was a problem hiding this comment.
A few comments – take them with a grain of salt though: After all I don't use these bindings myself.
I would be interested in @rprije's and @patrickmn's opinion though!
| | R1 { a :: () } | ||
| | R2 { x :: Double } | ||
| | R3 { a :: (), b :: () } | ||
| | R4 { x :: Double, y :: Double } |
There was a problem hiding this comment.
BTW I don't see any tests for simple record types like data D = D { a :: Bool, … } and newtypes – I assume they would be translated to records and also be influenced by the collapseSingletonRecords setting?!
There was a problem hiding this comment.
Did you see my comment above, @Gabriel439?
I'm mostly wondering whether these types are marshalled differently with the new default setting.
... based on feedback from @sjakobi This change the option to a three-valued option: * `Bare` - 1-field constructor does not include a nested record * `Wrapped` - 1-field constructor always includes a nested record * `Smart` - Named fields that don't begin with `_` include a nested record The default is `Wrapped` (for backwards compatibility), but users will probably want to eventually switch to `Smart`
|
|
||
| > < Foo : { x : Double } | Bar : { _1 : Double } > -- Wrapped | ||
|
|
||
| > < Foo : { x : Double } | Bar : Double > -- Smart |
| | R1 { a :: () } | ||
| | R2 { x :: Double } | ||
| | R3 { a :: (), b :: () } | ||
| | R4 { x :: Double, y :: Double } |
There was a problem hiding this comment.
Did you see my comment above, @Gabriel439?
I'm mostly wondering whether these types are marshalled differently with the new default setting.
There was a problem hiding this comment.
I only briefly looked over the code, but the approach looks great to me. Continuing with Wrapped for backwards compatibility for now while aiming to switch toward Smart seems like a good approach. Users might appreciate some explicit warning before that switch happens. On the other hand, maybe just mentioning it in the release notes is enough; I expect Dhall's user base knows to be pretty tolerant of backwards-incompatible changes at this point.
Thanks for making this change happen; Smart definitely looks like the sort of behaviour users are most likely to expect.
| instance (Selector s, Interpret a) => GenericInterpret (M1 S s (K1 i a)) where | ||
| genericAutoWith options@InterpretOptions{..} = do | ||
| let n :: M1 S s (K1 i a) r | ||
| n = undefined |
There was a problem hiding this comment.
I'm a little concerned about all these undefineds. Would Proxy be a safer alternative?
There was a problem hiding this comment.
@rprije: The undefineds are necessary. It's a limitation of the GHC generics API, since it doesn't support a Proxy-based selName.
... as suggested by @sjakobi
|
@sjakobi: After this change, all fields will be marshalled the same as they were in version 1.26.0 (i.e. |
This builds on top of #1315 to minimize disruption by disabling the
breaking change by default and instead requiring the user to opt in
by setting a new
collapseSingletonRecordsoption toTrue.The additional tests added to verify this also caught a bug in the
Interpretinstance for functions, which this change also fixes.