On Tue, Aug 18, 2009 at 23:03:16 +0200, Reinier Lamers wrote: > Here's a review. It have not actually tested that this code produces the > correct output, just checked that the code is readable and that its general > approach is sound. There are a few questions but no big problems. If you'd > like to push without renaming 'summarize', go ahead (or ask me to do it :-)).
Thanks! I'll work on this some more, amending if possible, or slapping on more code if I'm being diligent. > May I ask why you decided to refactor this code, and not refactor other > code, or fix something from the bug tracker? Hard to say. "I felt like it?" I think I was trying to understand Lele's issue1519 better when a comment he made made me realise that was potentially just a superficial bug and not something deep down in the core (coalescing and sorting, etc). I confess that a lot of my actions (triaging bugtrackers, writing code) are based on impulse and obsession rather than reason and calm. Doing this certainly felt good, but it really would have been better to go do work instead. :-) > > +-- | High-level representation of a piece of patch summary > > +data SummChunk = SummChunk FileName SummDetail ConflictState > > + deriving (Ord, Eq) > > + > > +data SummDetail = SummAddDir > > + | SummRmDir > > + | SummFile SummOp Int Int Int > > + | SummNone > > + deriving (Ord, Eq) > > + > > +data SummOp = SummAdd | SummRm | SummMod deriving (Ord, Eq) > > This still allows for combination of things that don't belong together > AFAICS. Those integers only make sense with hunk patches, not with add or > remove. Why not pull them into the SummOp type? Like: > > data SummDetail = SummAddDir > | SummRmDir > | SummFile SummOp > | SummNone > data SummOp = SummAdd | SummRm | SummMod Int Int Int Ah, in fact, my first attempt didn't even have a SummOp (SummFile was SummMod), but then I realised we were trying to combine SummAdd and SummHunk, presumably so that we could say something like A ./foo +32 We don't actually do this though, but it's conceivable that we might in the future. Perhaps this is another case of 'You Ain't Gonna Need It'? > > + s2 :: IsConflictedPrim -> [SummChunk] > > + s2 (IsC c x) = map (\(f,d) -> SummChunk f d c) $ s x > > + s :: Prim C(x y) -> [(FileName,SummDetail)] > > + s (FP f (Hunk _ o n)) = [(f,SummFile SummMod (length o) (length > > n) 0)] > > + s (FP f (Binary _ _)) = [(f,SummNone)] > > + s (FP f AddFile) = [(f,SummFile SummAdd 0 0 0)] > > + s (FP f RmFile) = [(f,SummFile SummRm 0 0 0)] > > + s (FP f (TokReplace _ _ _)) = [(f,SummFile SummMod 0 0 1)] > > + s (DP d AddDir) = [(d,SummAddDir)] > > + s (DP d RmDir) = [(d,SummRmDir)] > > I liked yesterday's version of this paragraph better. Those tuples distract > a bit from what the code really does. Those filenames are just being passed > on, they get a bit much of attention now. But I'm getting philosophical > here, there's nothing really *wrong* about coding it this way. I agree with you. I had moved the FileName up to SummDetail because I wanted sorting by filename to be for free, but now I think it's better to have more straightforward types like (SummAddDir d) > > + combine (x1@(SummChunk f1 d1 c1) : x2@(SummChunk f2 d2 c2) : ss) > > + | f1 == f2 = case combineDetail d1 d2 of > > + Nothing -> x1 : combine (x2:ss) > > + Just d3 -> combine $ SummChunk f1 d3 > > (combineConflitStates c1 c2) : ss > > + combine (x:ss) = x : combine ss > > combine [] = [] > > There's some untangling going on here, good thing. Though this one also > forms a challenge to someone from the > explicit-recursion-is-the-goto-of-functional-programming sect: how does one > rewrite this in a clearer way without using explicit recursion? I don't see > it with my tired head. Yeah. Anytime I'm potentially dealing with more than one element of the list I'm traversing I go straight to explicit recursion. > > hunk ./src/Darcs/Patch/Viewing.hs 183 > > + -- > > + combineDetail (SummFile o1 r1 a1 x1) (SummFile o2 r2 a2 x2) = > > + do o3 <- combineOp o1 o2 > > + return $ SummFile o3 (r1 + r2) (a1 + a2) (x1 + x2) > > + combineDetail _ _ = Nothing > > + -- > > What are those comment signs for? The comment signs are spacers. I don't want to use just whitespace because I tend to find that it makes it hard to understand the relationship between all the helper functions and their parent (but maybe I'm just being silly). I can refrain from doing this in the future as it's just a personal tic. > And where do you handle the case where one > of the counts of added or removed lines is -1? I simply don't model that. Hmm, maybe some sort of Nat type that just never goes below zero would be great. Oh, but I guess Num isn't that granular. One possibility might be to replace the Summ constructors which functions that error on zero. > > gen_summary :: Bool -> [IsConflictedPrim] -> Doc > > gen_summary use_xml p > > - = vcat themoves > > - $$ vcat themods > > - where themods = map summ $ combine $ sort $ concatMap s2 p > > + = vcat themods > > + where themods = map summ $ combine $ concatMap s2 p > > As for code quality, nothing wrong. I don't know if there are people out > there who rely on the sortedness of output, if so, this may not be such a > good idea. Yep, this is my fear. I don't know if this is subtly wrong or not. I'm willing to go for a "try and see" approach... > > xml_summary :: (Effect p, Patchy p, Conflict p) => Named p C(x y) -> Doc > > xml_summary p = text "<summary>" > > - $$ gen_summary True (conflictedEffect $ patchcontents p) > > + $$ (vcat . map summChunkToXML . gen_summary . > > conflictedEffect . patchcontents $ p) > > $$ text "</summary>" > > So xml_summary is the xml variant of summarize? Looks like it. I also wonder if we can move these out of Darcs.Patch.Viewing > Why not rename summarize to plain_summary or plaintext_summary or > line_summary if we're going for the ultimate in stylized code? I'm open to the idea. -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
pgp6ujEqZjwI3.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
