-
Notifications
You must be signed in to change notification settings - Fork 6
WIP main: factor the top-level as textToYAML #20
Conversation
cec71d8
to
6223361
Compare
Perhaps simplify the utility to only encompass this logic: codeToValue :: Text -> Text -> IO Value
codeToValue name code = do
expr <- case Dhall.Parser.exprFromText (Directed name 0 0 0 0) code of
Left err -> Control.Exception.throwIO err
Right expr -> return expr
expr' <- Dhall.Import.load expr
case Dhall.TypeCheck.typeOf expr' of
Left err -> Control.Exception.throwIO err
Right _ -> return ()
case Dhall.JSON.dhallToJSON expr' of
Left err -> Control.Exception.throwIO err
Right json -> return json Then if the user wants detailed error messages they can wrap that in |
8ad6cb4
to
d031a22
Compare
@Gabriel439, does that one look better? |
src/Dhall/YAML.hs
Outdated
@@ -0,0 +1,20 @@ | |||
{-# LANGUAGE DeriveDataTypeable #-} |
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 don't think you need this module any longer. All it does is re-export codeToValue
from the Dhall.JSON
module
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.
Right, removed it.
dhall-to-yaml/Main.hs
Outdated
then Dhall.JSON.omitNull json | ||
else json | ||
Data.ByteString.putStr (Data.Yaml.encode filteredJSON) )) | ||
Data.ByteString.putStr =<< (Data.Yaml.encode . omittingNull <$>) . explaining . (Dhall.JSON.codeToValue "(stdin)") |
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'd prefer to use do
notation here to split this up over multiple lines. This is difficult to read
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.
Done!
49072b5
to
ad57699
Compare
Added a provisional docstring to |
dhall-json.cabal
Outdated
aeson >= 1.0.0.0 && < 1.4 , | ||
dhall >= 1.11.0 && < 1.12, | ||
text >= 0.11.1.0 && < 1.3 , | ||
unordered-containers < 0.3 | ||
trifecta >= 1.6 && < 1.8 , |
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 think you can now remove the trifecta
dependency from the dhall-to-json
and dhall-to-yaml
executables
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.
Done.
src/Dhall/JSON.hs
Outdated
-} | ||
codeToValue :: Data.Text.Text -> Data.Text.Text -> IO Value | ||
codeToValue name code = do | ||
let textBS = Data.ByteString.UTF8.fromString . Data.Text.unpack |
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.
Oops! Perhaps I should have suggested that the first argument have type ByteString
that way we don't need to convert from Text
using the utf8-string
package
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.
Done.
dhall-json.cabal
Outdated
trifecta >= 1.6 && < 1.8 , | ||
unordered-containers < 0.3 , | ||
utf8-string , | ||
yaml >= 0.5.0 && < 0.9 |
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 think the library section doesn't need to depend on the yaml
package any longer now that codeToValue
returns a Value
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.
Done.
src/Dhall/JSON.hs
Outdated
{-| Convert a piece of Text carrying a Dhall inscription to an equivalent JSON Value | ||
|
||
>>> :set -XOverloadedStrings | ||
>>> :set -XOverloadedLists |
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 think this example doesn't require -XOverloadedLists
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.
Done.
src/Dhall/JSON.hs
Outdated
>>> Dhall.JSON.codeToValue "(stdin)" "{ a = 1 }" | ||
>>> Object (fromList [("a",Number 1.0)]) | ||
-} | ||
codeToValue :: Data.Text.Text -> Data.Text.Text -> IO Value |
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.
Don't forget to document what each argument represents
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.
Done.
e393ba3
to
212f2c6
Compare
All done and CI passes : -) |
Looks great! Thanks for doing this :) |
Thank you for your feedback! |
You're welcome! :) |
@Gabriel439, this is a suggested refactoring -- would you feel like including something like this?
If so, what improvements (besides the obvious lack of a docstring) would you suggest?