New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify Inject
/Interpret
for 1-field records
#1315
Conversation
Fixes #346 Now a Haskell type like: ```haskell data Example = Foo Bool | Bar Text ``` ... corresponds to this Dhall type: ```dhall { Foo : Bool | Bar : Text } ```
... as suggested by @sjakobi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! This looks like a very nice improvement! 👍
dhall/tests/Dhall/Test/Dhall.hs
Outdated
@@ -161,56 +161,62 @@ data NonEmptyUnion = N0 Bool | N1 Natural | N2 Text | |||
data Enum = E0 | E1 | E2 | |||
deriving (Eq, Generic, Inject, Interpret, Show) | |||
|
|||
data Mixed = M0 Bool | M1 | M2 () | |||
data Mixed = M0 Bool | M1 | M2 () | M3 Bool () | M4 { x :: Double, y :: Double } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there are still more interesting cases like
| M5 { x :: Text }
and
| M6 { x :: () }
Handling ()
fields consistently seems a bit tricky, but I don't think it's particularly important.
* 0, 1, or 2 fields * Fields of type `()` or `Double` * Named or anonymous fields
Ah great, I was very surprised by the implicit |
I think this broke a place where I'm deserializing a one-field record: data MyRecord = MyRecord { foo :: Text } myRecord = { foo : Text } -->
Just making sure that's intentional. |
@patrickmn: It was intentional, but we can make this behavior configurable via |
I understand it now, the error was just a bit puzzling until I looked at the recent merges. My data type was just a placeholder, so it's no bother :) |
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.
@patrickmn: I decided to disable the breaking change by default, at least for the upcoming release, by guarding it behind an option. See: #1321 |
* Disable 1-field simplification by default 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. * Change to three-valued option ... 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` * Don't depend on `fieldModifier` for determining if a field is anonymous ... as suggested by @sjakobi
Fixes #346
Now a Haskell type like:
... corresponds to this Dhall type: