[#3] Implement basic TOML parser #12

chshersh · 2018-04-03T21:27:02Z

This PR introduces basic implementation of TOML parser. Also, some types in Toml.Type module were refactored. This resulted in a lot of changes... Though, it works! You can checkout parseable example in test.toml file.

chshersh · 2018-04-03T21:34:52Z

Argh, -XDerivingStrategies doesn't work on GHC-8.0.2... I guess I need to drop them in that case...

vrom911

Looks cool! That's hell a huge work 👏
I've added a couple of comments and some questions..

vrom911 · 2018-04-03T22:10:02Z

src/Toml/Parser.hs

       ) where

+-- I hate default Prelude... Do I really need to import all this stuff manually?..


😆 give this man an universum

vrom911 · 2018-04-03T22:11:45Z

src/Toml/Parser.hs

@@ -1,13 +1,184 @@
 module Toml.Parser


Probably write some module description for haddock

vrom911 · 2018-04-03T22:35:58Z

src/Toml/Parser.hs

+bareKeyP = lexeme $ Text.pack <$> bareStrP
+  where
+    bareStrP :: Parser String
+    bareStrP = some $ alphaNumChar <|> char '_' <|> char '-'


Cool that it could start with any of these symbols (at least I didn't see any restriction on it in the docs).
Also instead of char '_' <|> char '-' we can use oneOf ['_', '-'] 🙂

Yeah, restricting to something like - can't be the first character will complicate things a lot... Though, -_-_-_- is a valid key in TOML, I hope people won't use such keys...

Oops, I forgot to use oneOf ['_', '-'] there... I guess I can silently fix this under one of the future PR :pepe:

vrom911 · 2018-04-03T22:40:22Z

src/Toml/Parser.hs

+    bareStrP = some $ alphaNumChar <|> char '_' <|> char '-'
+
+stringP :: Parser Text
+stringP = lexeme $ Text.pack <$> (char '"' *> anyChar `manyTill` char '"')


As I understand it also could be between '? Should we try it also here?

Well, I'm not sure how to write such parser properly... Because ' is for literal strings. With ' I can write:

quoted = 'Tom "Dubs" Preston-Werner'

But with " this needed to be write like this:

quoted = "Tom \"Dubs\" Preston-Werner"

Actually, I'm not sure which behavior is currently implemented and what be the result of this parser... In "" strings I also need to support this:

\b - backspace (U+0008) \t - tab (U+0009) \n - linefeed (U+000A) \f - form feed (U+000C) \r - carriage return (U+000D) \" - quote (U+0022) \\ - backslash (U+005C) \uXXXX - unicode (U+XXXX) \UXXXXXXXX - unicode (U+XXXXXXXX)

Not sure how to do this easilty and what comes out of the box...

Okay, "Tom \"Dubs\" Preston-Werner" is not even parseable because current parser interprets \ as a separate character which makes sense. So I just need to replace " with ' to have parser of literal strings.

vrom911 · 2018-04-03T22:43:50Z

src/Toml/Parser.hs

+    <|> True  <$ text "true"
+
+-- dateTimeP :: Parser DateTime
+-- dateTimeP = error "Not implemented!"


That would be tough one..

Yeah, Will create separate issue for this...

vrom911 · 2018-04-03T22:48:52Z

src/Toml/Parser.hs

+    k <- keyP
+    text_ "="
+    uval <- valueP
+    case typeCheck uval of


This is cool decision to make it on this level! 👍

vrom911 · 2018-04-03T22:59:57Z

src/Toml/Parser.hs

+    isThPref = isPrefixOf `on` unKey . thName
+
+isPrefixOf :: Eq a => NonEmpty a -> NonEmpty a -> Bool
+(x :| xs) `isPrefixOf` (y :| ys) = x == y && List.isPrefixOf xs ys


Isn't this the function from Data.List.NonEmpty?

Oh, I see it's not, it works with simple list as prefix 🤔

@vrom911 I was also surprised by this behavior. There's:

isPrefixOf :: [a] -> [a] -> Bool isPrefixOf :: [a] -> NonEmpty a -> Bool

But somehow there's no

isPrefixOf :: NonEmpty a -> NonEmpty a -> Bool

🤔 🤷‍♂️

vrom911 · 2018-04-03T23:04:47Z

test.toml

+  2Inner = +42
+
+[table.name.1]
+  listInner."google.com" = [true, false]


Sorry, I've seen this red highlight in toml repo also, why is that?

I don't know. universum also had it. I guess it's just a github bug. Though I didn't google it...

vrom911 · 2018-04-03T23:06:41Z

tomland.cabal

@@ -26,13 +26,16 @@ library

  build-depends:       base >= 4.9 && < 5
                     , hashable
+                     , megaparsec
+                     , parser-combinators


Why is this needed?

parser-combinators is a dependency for megaparsec. Some parser combinators are not exported by megaparsec. Instead more general versions of them are in the parser-combinators packages. Specifically, I need these ones:

sepBy1 :: MonadPlus m => m a -> m sep -> m (NonEmpty a) between :: Applicative m => m open -> m close -> m a -> m a sepBy :: Alternative m => m a -> m sep -> m [a] manyTill :: Alternative m => m a -> m end -> m [a]

vrom911

Looks cool! I only wanted to ask about will we continue to update changelog if it's only the initial version.. If yes then we can add a few words there, but I' not sure..

vrom911 · 2018-04-04T13:10:11Z

src/Toml/Type.hs

@@ -122,7 +120,9 @@ arr6 = [ 1, 2.0 ] # INVALID
    -}
    Array  :: [Value t] -> Value 'TArray

-- | Untyped 'Value'.
+-- TODO: move into Toml.Type.Internal module then?..


That's good point, because it's exported at the moment..

chshersh · 2018-04-04T13:20:14Z

@vrom911 Thanks for your review! I guess we should update changelog and reflect every issue we did in there. Will write it. Also I can move UValue into separate module.

vrom911

That looks great! 👍

vrom911 · 2018-04-04T15:47:32Z

src/Toml/Type.hs

@@ -120,7 +120,9 @@ arr6 = [ 1, 2.0 ] # INVALID
    -}
    Array  :: [Value t] -> Value 'TArray

-- | Untyped 'Value'.
+-- TODO: move into Toml.Type.Internal module then?.. But it uses 'DateTime' which is not internal...


[#3] Implement basic TOML parser

acc662b

chshersh added the parser Everything related to `Text -> Toml` label Apr 3, 2018

chshersh self-assigned this Apr 3, 2018

chshersh requested a review from vrom911 April 3, 2018 21:27

vrom911 reviewed Apr 3, 2018

View reviewed changes

Fix documentation and Travis CI build

b637adc

vrom911 reviewed Apr 4, 2018

View reviewed changes

chshersh added 2 commits April 4, 2018 16:41

Update CHANGELOG

3a28102

Fix Travis CI for GHC-8.0.2

ae539dd

vrom911 approved these changes Apr 4, 2018

View reviewed changes

chshersh merged commit f087a93 into master Apr 4, 2018

chshersh deleted the chshersh/3-parser branch April 4, 2018 15:53

chshersh mentioned this pull request Apr 4, 2018

Add parsing of literal strings #16

Closed

chshersh mentioned this pull request May 20, 2018

Support full-featured string parser #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3] Implement basic TOML parser #12

[#3] Implement basic TOML parser #12

chshersh commented Apr 3, 2018

chshersh commented Apr 3, 2018

vrom911 left a comment

vrom911 Apr 3, 2018

vrom911 Apr 3, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

chshersh Apr 4, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

chshersh Apr 4, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

vrom911 Apr 3, 2018

vrom911 Apr 3, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

vrom911 Apr 3, 2018

chshersh Apr 3, 2018

vrom911 left a comment

vrom911 Apr 4, 2018

chshersh commented Apr 4, 2018

vrom911 left a comment

vrom911 Apr 4, 2018

		) where

		-- I hate default Prelude... Do I really need to import all this stuff manually?..

[#3] Implement basic TOML parser #12

[#3] Implement basic TOML parser #12

Conversation

chshersh commented Apr 3, 2018

chshersh commented Apr 3, 2018

vrom911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrom911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chshersh commented Apr 4, 2018

vrom911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment