-
Notifications
You must be signed in to change notification settings - Fork 321
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
[acc-errors] Accumulate errors for Seq and Vector parsers #600
base: acc-errors
Are you sure you want to change the base?
Conversation
Data/Aeson/Types/Internal.hs
Outdated
|
||
accumulateTraverseVector :: (a -> Parser b) -> Vector a -> Parser (Vector b) | ||
accumulateTraverseVector f v = | ||
if V.null v |
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 wonder, if it makes sense to define
newtype AccParser a = AccParser { runAccParser :: Parser a }
instance Applicative AccParser where
AccParser f <*> AccParser x = AccParser (f <*>+ x) -- !!!
and use it, and not re-implement traverse
for everything.
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 agree, it's a good idea to use Traversable
here. Reexporting Parser
versions that use AccParser
under the hood is nice too.
I'm not sure when I'll get around to playing more with this, so feel free to steal the commit if anyone wants to keep going before I get the chance. |
b506cfd
to
ded0a7d
Compare
ded0a7d
to
09703bb
Compare
I changed this to use an |
09703bb
to
9eef7e7
Compare
61ab5a9
to
584135e
Compare
Naming has room for improvements, maybe we should even have a module for accumulation related functions, or should we do this in some other way like using a newtype so we can have proper traversable instances (not sure if that'll work out)?
Ping @Lysxia