Hi, Here's the review. I have some nitpicks on what I consider good style for comments. I think that we should not allow questions in comments, and instead try to find answers to such questions immediately. But I haven't read piles of software project managements books, so maybe someone who has can correct me.
Regards, Reinier On Wednesday 22 April 2009 17:30:25 [email protected] wrote: >hunk ./src/Darcs/Patch/Permutations.hs 158 >+-- | 'headPermutationsFL' @p:>:ps@ returns all the permutations of the list >+-- in which one element of @ps@ is commuted past @p@ >+-- >+-- Suppose we have a sequence of patches >+-- >+-- > X h a y s-t-c k >+-- >+-- Suppose furthermore that the patch @c@ depends on @t@, which in turn >+-- depends on @s...@. This function will return >+-- >+-- > X :> h a y s t c k >+-- > h :> X a y s t c k >+-- > a :> X h y s t c k >+-- > y :> X h a s t c k >+-- > s :> X h a y t c k >+-- > k :> X h a y s t c Nice :-). >+ -- | There is no difference between this and the 'commute' function >+ -- other than the order. ('commutex' uses '(:<)' and 'commute' >+ -- uses '(:>)'. >+ -- >+ -- Question: if there is no difference, why do we have two different >+ -- functions? Is it just convenience? Eric 2009-04-19 I believe that source code comments are not the right medium to ask questions. If no current darcs hacker knows an answer to this, we should ask David. >+-- | Patches whose concrete effect which can be expressed as a list of >+-- primitive patches. >+-- >+-- A minimal definition would be either of @effect@ or @effec...@. Nice, learned something there. >+-- | 'addP' @x cy@ commutes @x@ past @cy@ if possible. >+-- If it's not possible, it prepends @x@ co the context @c@ >+-- In either case, it returns the modified @c...@. > addP :: (Patchy p, ToFromPrim p) => p C(x y) -> Non p C(y) -> Non p C(x) > addP p n | Just n' <- p >* n = n' > addP p (Non c x) = Non (p:>:c) x This comment might be clearer about whether or not x is included in the result (afaics it is in both cases). Eg. 'This function returns @Non xc y@ if @x@ and @c@ do not commute, and returns @Non cx y@ otherwise'. >hunk ./src/Darcs/Patch/Non.hs 100 > >+-- | 'addPs' @xs cy@ commutes as many patches of @xs@ past @cy@ as >+-- possible, stopping at the first patch that fails to commute. >+-- Note the fact @xs@ is a 'RL' >+-- >+-- Suppose we have >+-- >+-- > x1 x2 x3 [c1 c2 y] >+-- >+-- and that in our example @c1@ fails to commute past @x1@, this >+-- function would commute down to >+-- >+-- > x1 [c1'' c2'' y''] x2' x3' >+-- >+-- and return @[x1 c1'' c2'' y'']@ Why are you using c1 and c2 here, instead of just a single c? >[Haddock some simple functions in Darcs.Patch.Real. >Eric Kow <[email protected]>**20090419151449 > Ignore-this: fd05c99677687056042dcee649dbe2b6 > The only thing in common in particular being that these are relatively > straightforward to understand: is_duplicate, sealed2non and allNormals. >] hunk ./src/Darcs/Patch/Real.hs 88 > Conflictor :: [Non RealPatch C(x)] -> FL Prim C(x y) -> Non RealPatch > C(x) -> RealPatch C(y x) InvConflictor :: [Non RealPatch C(x)] -> FL Prim > C(x y) -> Non RealPatch C(x) -> RealPatch C(x y) > >+-- | Either a 'Duplicate' or 'Etacilpud' patch I'd prefer a full sentence here... "'is_duplicate' returns true if the only argument is either a 'Duplicate' or 'Etacilpud' patch". >+-- | If @xs@ consists only of 'Normal' patches, 'allNormal' @xs@ returns >+-- @Just pxs@ those patches (so @lengthFL pxs == lengthFL xs@) I'd expect the comment to also state what the function returns if the patches are not all normal.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
