On Sat, Oct 23, 2010 at 21:01:34 +0000, Ganesh Sittampalam wrote: > Fri Oct 22 06:49:00 BST 2010 Ganesh Sittampalam <gan...@earth.li> > * don't silently throw away remaining parse input > > Fri Oct 22 06:53:59 BST 2010 Ganesh Sittampalam <gan...@earth.li> > * drop the unused and often ignored "want eof" parameter to readPatch' > > Fri Oct 22 06:58:29 BST 2010 Ganesh Sittampalam <gan...@earth.li> > * drop the nested Maybes in the patch parser
This looks good. I hope there wasn't anything subtle/tricky in there,
because I certainly didn't spot it.
I reviewed the three and then glanced through the full diff with
conflict resolution. All pushed.
don't silently throw away remaining parse input
-----------------------------------------------
These are the active ingredients of the patch (Darcs.Patch.Read)
> -readPatch :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )),
> B.ByteString)
> -readPatch ps = case parseStrictly (readPatch' False) ps of
> - Just (Just p, ps') -> Just (p, ps')
> - _ -> Nothing
> +readPatchPartial :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )),
> B.ByteString)
> +readPatchPartial ps
> + = case parseStrictly (readPatch' False) ps of
> + Just (Just p, ps') -> Just (p, ps')
> + _ -> Nothing
Just a rename and whitespace change.
> hunk ./src/Darcs/Patch/Read.hs 59
> +readPatch :: ReadPatch p => B.ByteString -> Maybe (Sealed (p C(x )))
> +readPatch ps
> + = case readPatchPartial ps of
> + Just (p, ps') | B.null (dropSpace ps') -> Just p
> + _ -> Nothing
And the new behaviour. Before we were using readPatchPartial everywhere
(sigh!) and now we're a bit more careful. In cases where we aren't
expecting leftovers other than whitespace, parsing should fail if we do
spot it.
I'm sort of thinking of this patch as correcting for confirmation bias
("I'm thinking of a rule. The sequence 2, 4, 6 follows my rule..."
http://www.devpsy.org/teaching/method/confirmation_bias.html ). [Or
maybe there's some other psych phenomenon that's more appropriate here].
So now instead of just being happy the parser succeeds, we're also more
carefully making sure it isn't succeeding for the wrong reasons.
...
For the most part, the rest of this patch switches over to readPatch.
(If we're wrong here, the consequences will be darcs complaining about
not being able to read some patch file). I didn't put any thought into
making sure we weren't overshooting though, sorry! It seemed to make
sense at a glance.
The two cases where we still let ourselves using readPatchPartial are
Darcs.Patch.Bundle (that really needs to be renamed PatchBundle)
getPatches and parsePatches, which I guess makes sense because patch
bundles have lots of stuff after the patches themselves.
drop the unused and often ignored "want eof" parameter to readPatch'
--------------------------------------------------------------------
Skipping down to the active ingredients (and ignoring the
straightforward consequences of said ingredients)
> instance ReadPatch p => ReadPatch (Braced p) where
> - readPatch' eof =
> - do mps <- bracketedFL (readPatch' False) '{' '}'
> + readPatch' =
> + do mps <- bracketedFL readPatch' '{' '}'
> case mps of
> hunk ./src/Darcs/Patch/Read.hs 69
> - Just (Sealed ps) -> do when eof lexEof
> - return $ Just $ Sealed $ Braced ps
> - Nothing -> liftM (fmap (mapSeal Singleton)) $ readPatch' eof
> + Just (Sealed ps) -> return $ Just $ Sealed $ Braced ps
> + Nothing -> liftM (fmap (mapSeal Singleton)) $ readPatch'
There's little bits and pieces of code in these parsers like that which
optionally eats whitespace + eof... only we *never* use it.
All the readPatch' calls either thread their want_eof through, or just
pass False. Maybe YAGNI violation, maybe cruft, doesn't matter. Gone!
drop the nested Maybes in the patch parser
------------------------------------------
This one was a little trickier for me to review, but it seems good.
I'm always happier to see code become simpler/more transparent.
(Wikibooks reminded me of MonadPlus)
> instance (ReadPatch p, PatchListFormat p) => ReadPatch (PatchInfoAnd p) where
> - readPatch' =
> - do x <- readPatch'
> - case x of
> - Just (Sealed p) -> return $ Just $ Sealed $ n2pia p
> - Nothing -> return Nothing
> + readPatch' = mapSeal n2pia <$> readPatch'
I'll ignore the bits of the code like this which are just consequences
of less double-layer Maybe bureaucracy to deal with
> hunk ./src/Darcs/Patch/Read.hs 53
> readPatch'
> - :: ParserM m => m (Maybe (Sealed (p C(x ))))
> + :: ParserM m => m (Sealed (p C(x )))
Here's our big change. I'm slightly concerned about inner Nothing
meaning something important, but it looks like we address that below.
> instance ReadPatch p => ReadPatch (Braced p) where
> readPatch' =
> - do mps <- bracketedFL readPatch' '{' '}'
> + do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return Nothing
Here's the whole thing again
| instance ReadPatch p => ReadPatch (Braced p) where
| readPatch' =
| do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return Nothing
| case mps of
| Just (Sealed ps) -> return $ Sealed $ Braced ps
| Nothing -> mapSeal Singleton <$> readPatch'
OK, now this is a more interesting bit, not just bureaucracy removal.
The way I currently understand it is
BEFORE: functions like bracketedFL used to give (Just Nothing) on
failure rather than Nothing, which I sort of read as "it's OK
for this parsing to fail; we'll just tell you it happened
instead of stopping".
AFTER: such functions just give us plain old Nothing
However the code above still assumes we're using this permission-to-fail
model and doing something on said failure (ie. handling Nothing). So we
basically we have to restore this permission-to-fail by giving return
Nothing as an alternative to the parser succeeding.
So
1) ps <- bracketedFL readPatch' ...
- just failing if bracketedFL fails, bad
2) mps <- Just <$> bracketedFL readPatch' ...
- worse, now we're pretending we handle the failure
3) mps <- Just <$> bracketedFL readPatch ... <|> return Nothing
- much better
If I've understood correctly, the following formulations should be equivalent.
|A| instance ReadPatch p => ReadPatch (Braced p) where
|A| readPatch' =
|A| do mps <- (Just <$> bracketedFL readPatch' '{' '}') <|> return
Nothing
|A| case mps of
|A| Just (Sealed ps) -> return $ Sealed $ Braced ps
|A| Nothing -> mapSeal Singleton <$> readPatch'
|B| instance ReadPatch p => ReadPatch (Braced p) where
|B| readPatch' =
|B| (mapSeal Braced <$> bracketedFL readPatch' '{' '}')
|B| <|> (mapSeal Singleton <$> readPatch')
Not actually proposing B (though it does seem a bit nicer)
Just cross-checking to make sure I've understood.
> hunk ./src/Darcs/Patch/Read.hs 100
> - peekforc pre bfl (return Nothing)
> + peekforc pre bfl mzero
I think that's one example of withdrawing that permission-to-fail
> = skipSpace >> choice
> [ return' $ readHunk x
> hunk ./src/Darcs/Patch/Read.hs 133
> , return' $ readTok x
> , return' $ readBinary x
> , return' readIdentity
> - , liftM Just $ readSplit x
> + , readSplit x
> , return' $ readChangePref
> hunk ./src/Darcs/Patch/Read.hs 135
> - , return Nothing
And that's another case
The rest of it is just more of the same.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:ko...@jabber.fr (Jabber or Google Talk only)
pgpqoI1gV7Ce4.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
