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! |
bergmark
left a comment
bergmark
left a comment
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
| 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.
| -- 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.
|
|
||
| -- | 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. |
4 participants
This defines a
liftP2accumulating errors from both branches. Resolves #578.