Skip to content
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

[#8] Create EDSL for tomland #55

Merged
merged 2 commits into from
Jun 3, 2018
Merged

[#8] Create EDSL for tomland #55

merged 2 commits into from
Jun 3, 2018

Conversation

vrom911
Copy link
Member

@vrom911 vrom911 commented Jun 3, 2018

Resolves #8

@vrom911 vrom911 added the ast Type of TOML label Jun 3, 2018
@vrom911 vrom911 self-assigned this Jun 3, 2018
@vrom911 vrom911 requested a review from chshersh June 3, 2018 08:55
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, the implementation is really clean and simple! And it helped to reduce amount of code for manually creating TOML objects! This will help us a lot in creating unit tests in future!

src/Toml/Edsl.hs Outdated
type Env = State TOML ()

toml :: Env -> TOML
toml env = execState env $ TOML mempty mempty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TOML mempty mempty appears multiple times across code... Maybe it's a good idea to create variable emptyToml?

src/Toml/Edsl.hs Outdated
toml env = execState env $ TOML mempty mempty

(=:) :: Key -> Value a -> Env
(=:) k v = modify $ \tml -> tml {tomlPairs = HashMap.insert k (AnyValue v) (tomlPairs tml)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this code has something in common with code in Bi.Combinators. Maybe you could write function with type like MonadSate TOML m => ... and reuse in all places?..

src/Toml/Edsl.hs Outdated

table :: Key -> Env -> Env
table k env = modify $ \tml -> tml
{ tomlTables = Prefix.insert k (execState env (TOML mempty mempty)) (tomlTables tml)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execState env (TOML mempty mempty) looks like toml function you defined above!

"list" =: Array [String "one", String "two"]
"time" =: Array [Date $ Day (fromGregorian 2018 3 29)]
table "table.name.1" $ do
"aInner" =: Int 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I know how you can write key =: 1 instead of ke =: Int 1. Since we have GADT for Value, you can implement Num instance for Value 'TInt. And this will guarantee that it will be converted to Value using Int constructor. Also all pattern matchings will be exhaustive inside instance implementation because of that!

Same for IsString instance for Value. Not sure about IsList intsance, it probably will be impossible to implement... And, unfortunately, there's no IsBool type class in Haskell 😞

src/Toml/Edsl.hs Outdated
import qualified Data.HashMap.Strict as HashMap
import qualified Toml.PrefixTree as Prefix

type Env = State TOML ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to name this type somehow else? Since it EDSL for creating toml, we could name it like TomlDsl or Tdsl or TDSL or something like that.

@@ -49,7 +48,7 @@ main = do
content <- TIO.readFile "test.toml"
case parse content of
Left (ParseException e) -> TIO.putStrLn e
Right toml -> TIO.putStrLn $ prettyToml toml
Right tml -> TIO.putStrLn $ prettyToml tml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe toml is not the best name since if you export toml function you can't use toml as variable name? Maybe it's better to call it mkToml or declareTolm or defineToml to constructToml or createToml or something else? What do you think?

@chshersh chshersh merged commit 1f67b3d into master Jun 3, 2018
@chshersh chshersh deleted the vrom911/8-edsl branch June 3, 2018 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast Type of TOML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants