The problem is the usage of many
in parseJNumber
. It is also a valid parse, when no character of the following string is consumed ("many p applies the parser p zero or more times. [...]"). What you need is many1
:
parseJNumber = do
num <- many1 (oneOf "0123456789")
return $ JNumber (read num :: Double)
Edit:
Somehow, I think your combination of (.)
and ($)
looks kind of weird. I use (.) when I can get rid of a function parameter (like in the usage of (>>=)
) and ($) when I'm to lazy to write parentheses. In your function parseJString
you do not need (.)
in order to get the right binding precedences. (I did the same transformation in the code above.)
parseJString = do
str <- between (char '"') (char '"') (many (noneOf "\""))
return $ JString str
Additionally, you could eliminate code-repetition by refactoring parseJBool
:
parseJBool = do
val <- string "true" <|> string "false"
return (case val of
"true" -> JBool True
"false" -> JBool False)
I would even rewrite the case-construct into a (total) local function:
parseJBool = (string "true" <|> string "false") >>= return . toJBool
where
-- there are only two possible strings to pattern match
toJBool "true" = JBool True
toJBool _ = JBool False
Last but not least, you can easily transform your other functions to use (>>=)
instead of do-blocks.
-- additionally, you do not need an extra type signature for `read`
-- the constructor `JNumber` already infers the correct type
parseJNumber =
many1 (oneOf "0123456789") >>= return . JNumber . read
parseJString =
between (char '"') (char '"') (many (noneOf "\"")) >>= return . JString