Data instance for Expr. #418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes dhall-lang#416. Also adds some Template Haskell to be able to load & resolve a Dhall expression at compile-time. Without the `Data` instance, the AST had to be pretty-printed back to a `String` in the Template Haskell, and in 1.14 imports now auto-normalize, which often results in much larger ASTs, and therefore (at least with the current version) _much_ longer parses after re-serialization. Adding the `Data` instance avoids that. The current implementation of the `Data` instance removes `Data.Text.Lazy.Builder` in favor of using `Data.Text.Lazy` directly, this is simply because the latter already has a `Data` instance. It’s probably reasonable to instead have a non-derived `Data` instance for `Chunks` that explicitly uses `to`/`fromLazyText`, but that approach was less mechanical, so I punted on it.
|
Another thing I’d like to do with this is somehow have |
Gabriella439
reviewed
Jun 1, 2018
dhall.cabal
Outdated
| @@ -176,6 +176,7 @@ Library | |||
| prettyprinter >= 1.2.0.1 && < 1.3 , | |||
| prettyprinter-ansi-terminal >= 1.1.1 && < 1.2 , | |||
| scientific >= 0.3.0.0 && < 0.4 , | |||
| template-haskell , | |||
There was a problem hiding this comment.
Perhaps add an upper bound to template-haskell
src/Dhall/TH.hs
Outdated
|
|
||
| importDhallExpression :: Text.Text -> IO (D.Expr D.Src D.X) | ||
| importDhallExpression = | ||
| either (const (fail "invalid Dhall expression")) D.load |
There was a problem hiding this comment.
Perhaps include the name Dhall.TH.importDhallExpression in the error string so that it's easier for people to hunt down the source for where the error originates from
| @@ -0,0 +1,28 @@ | |||
| {-# LANGUAGE TemplateHaskell #-} | |||
|
|
|||
There was a problem hiding this comment.
Could you add a module header explaining an example use case for this functionality?
Gabriella439
approved these changes
Jun 4, 2018
| {-| Similar to `input`, but without interpreting the Dhall `Expr` into a Haskell | ||
| type. | ||
| -} | ||
| inputExpr |
There was a problem hiding this comment.
Very handy! Thanks for adding this :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #416.
Also adds some Template Haskell to be able to load & resolve a Dhall expression
at compile-time.
Without the
Datainstance, the AST had to be pretty-printed back to aStringin the Template Haskell, and in 1.14 imports now auto-normalize, which often
results in much larger ASTs, and therefore (at least with the current version)
much longer parses after re-serialization. Adding the
Datainstance avoidsthat.
The current implementation of the
Datainstance removesData.Text.Lazy.Builderin favor of usingData.Text.Lazydirectly, this issimply because the latter already has a
Datainstance. It’s probablyreasonable to instead have a non-derived
Datainstance forChunksthatexplicitly uses
to/fromLazyText, but that approach was less mechanical, so Ipunted on it.