Combinator for accumulative errors #592
Conversation
|
Hm. Maybe I shouldn't touch |
|
Very cool, thanks! I think I agree that we shouldn't change |
|
OK! |
There was a problem hiding this comment.
- I think we should have some tests for error accumulation by itself
- Does this affect performance?
I suppose one of the next steps would be to do this accumulation for built-in instances and TH/Generics? That should help us check the API and performance.
I'm thinking about how to release this and continue development... How about merging this into a branch first?
| @@ -8,6 +8,7 @@ import Prelude.Compat | |||
|
|
|||
There was a problem hiding this comment.
With the example name "Simplest" I think we should keep this file as it is, but we can add a SimplestWithErrorAccumulation instead and add comments/references in this file
| @@ -39,3 +40,6 @@ main = do | |||
| print req | |||
| let reply = Coord 123.4 20 | |||
| BL.putStrLn (encode reply) | |||
| let asCoord :: f Coord -> f Coord | |||
There was a problem hiding this comment.
For an example I think it would be clearer to skip asCoord and write a type annotation around verboseDecode instead.
| @@ -28,9 +29,9 @@ instance ToJSON Coord where | |||
| -- should match the format used by the ToJSON instance. | |||
|
|
|||
| instance FromJSON Coord where | |||
| parseJSON (Object v) = Coord <$> | |||
There was a problem hiding this comment.
Is it possible to write this in the f <op> x <op> y <op> z style? It might not be better but I'm curious!
There was a problem hiding this comment.
Sure! Coord <$> v .: "x" <*>+ v .: "y" should work.
| @@ -333,6 +340,22 @@ apP d e = do | |||
| return (b a) | |||
| {-# INLINE apP #-} | |||
|
|
|||
| -- | A variant of 'Control.Applicative.liftA2' that lazily accumulates errors | |||
| -- from both subparsers. | |||
| liftP2 :: (a -> b -> c) -> Parser a -> Parser b -> Parser c | |||
There was a problem hiding this comment.
I'm personally not a fan of the name liftA2, what do you think? Can you think of a better name?
There was a problem hiding this comment.
I can't think of a better name, but we could also just remove it. It is quite redundant with (<*>+).
|
I'd be fine with taking the time to see the performance implications in a separate branch. |
|
Cool let's do that, i'll file a PR against master to keep track of everything. |
|
Does anyone know why this code got removed? I've been looking for it 🥲 🥲 🥲 |
|
@fishtreesugar that doesn't quite clarify, as @Lysxia actually already merged his fixes before that. |
This defines a
liftP2accumulating errors from both branches. Resolves #578.