Parsers#3
Conversation
| test ∷ ∀ r. Eff (TestEffects r) Unit | ||
| test = do | ||
| Console.printLabel "\n\n:::::::::: TESTING Argonaut codecs ::::::::::\n\n" | ||
| testJsonSerialization |
There was a problem hiding this comment.
That's because of stack overflow in Gen
| >=> flip P.runParser do | ||
| res ← expr | ||
| eof | ||
| pure res |
| eof = do | ||
| input ← gets \(P.ParseState input _ _) → input | ||
| unless (A.null input) $ P.fail "Expected EOF" | ||
| pure unit |
There was a problem hiding this comment.
Unnecessary pure unit.
| p ← A.many do | ||
| o ← pb | ||
| r ← pa | ||
| pure $ o × r |
There was a problem hiding this comment.
I don't like this notation for binary operators. Why do you prefer it?
There was a problem hiding this comment.
Yeah, my point is just that if it's trivial applicative usage, it should be favored.
| a ← token | ||
| case a of | ||
| Identifier s → pure s | ||
| _ → P.fail "Token is not an identifier" |
There was a problem hiding this comment.
PC.try $ token >>= case _ of
Should we be putting try in top-level parsers or deferring to use site?
There was a problem hiding this comment.
👍
I tried to wrap parsers with wrap because it seemed a bit more convenient to me. Is there any best practices for parsing stuff?
There was a problem hiding this comment.
If you put try at the root of your parsers, you have no option to use it without try. I don't know if this flexibility is important, which is why I posed it as a question. I haven't written enough complex parsers to have a strong opinion. But it seems like you mostly want it in alternatives, and being able to tell in the alternative structure which is backtracking or not is helpful.
| derefExpr = PC.try do | ||
| e ← primaryExpression | ||
| modifiers ← A.many modifier | ||
| (mbWildcard ∷ Maybe t) ← PC.optionMaybe do |
There was a problem hiding this comment.
Unnecessary parens. Why is the type annotation necessary?
There was a problem hiding this comment.
Next expression is dropped. It has constrained type. Ps can't say what's that.
| operator "." | ||
| wildcard | ||
| let | ||
| modified = F.foldl (\a f → f a) e modifiers |
There was a problem hiding this comment.
I thought this syntax is deprecated. Cool!
| pure \e → embed $ Sig.Binop { op: Sig.IndexDeref, lhs: e, rhs }) | ||
|
|
||
| wildcard ∷ ∀ m t. (Corecursive t (Sig.SqlF EJ.EJsonF), Monad m) ⇒ P.ParserT (Array Token) m t | ||
| wildcard = operator "*" $> (embed $ Sig.Splice Nothing) |
There was a problem hiding this comment.
embed (Sig.Splice Nothing)
Must...use...$
| pure $ isNothing n | ||
| let inv = fromMaybe true mbInv | ||
| suffix ← betweenSuffix <|> inSuffix <|> likeSuffix | ||
| pure $ \e → (if inv then _NOT else id) $ suffix e |
There was a problem hiding this comment.
These $s are unnecessary when the rhs is a lambda.
| mbV ← | ||
| PC.optionMaybe definedExpr | ||
| sortStr ← | ||
| map (fromMaybe "asc") $ PC.optionMaybe (keyword "asc" <|> keyword "desc") |
There was a problem hiding this comment.
fromMaybe "asc" <$> ...
There was a problem hiding this comment.
One reason me preferring map is that I know precedence of $. E.g.
map (fromMaybe "asc") $ PC.optionMaybe $ foo <|> bar
| skipped r = PC.try do | ||
| res ← r | ||
| PS.skipSpaces | ||
| pure res |
|
@natefaubion @garyb good to go? |
@garyb @natefaubion Please review
This depends on slamdata/purescript-ejson#17
Initially I tried to reuse ejson parsing coalgebra, but it seems that such parser isn't parser in parsec meaning. But something different. So, I've ported quasar sql^2 parser with part of scala's stdparsers.
Also it has
Gencoalgebra that uses ejson's one.Search example removed. I can add link to SD sources if you guy think that it's for the best.
Couple things that can be improved
Expr afor anything that could be ranged/summed/etc.Sqlisn't safe by construction.