[darcs-users] darcs patch: Add an explanation of headPermutationsFL... (and 6 more)

Eric Kow kowey at darcs.net
Thu Apr 23 18:26:56 UTC 2009


On Wed, Apr 22, 2009 at 23:32:27 +0200, Reinier Lamers wrote:
> Here's the review. I have some nitpicks on what I consider good style for 
> comments.

Thanks! I'll work on amending the relevant patches (I've applied the
ones that pass review)

> 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.

I'd also be interested to see what the right approach is.

There was a thread on the mailing list where Florent submitted some
questions, and David commented that he would welcome personal
statements of uncertainty (i.e. signing them to make sure that this
doesn't add to the confusion):
  http://lists.osuosl.org/pipermail/darcs-users/2008-October/014303.html
and I've just sort of stuck with that.  I do agree that if we can
find the answers easily enough we should just avoid them, though.

> On Wednesday 22 April 2009 17:30:25 E.Y.Kow at brighton.ac.uk wrote:
> >+    --   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.

I've just changed this to say that the two variants exist for
readability only:
http://lists.osuosl.org/pipermail/darcs-devel/2007-September/006323.html

> >+-- | '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 @cy at .
> > 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'. 

I think if @x@ commutes past, we don't include it, but if it sticks
somewhere, we do.  I don't really quite get this stuff yet, though,
so I'll probably revisit it on holiday next week .

> >+--   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?

Huh, I don't know why.  I can see why I used multiple x's (so that we
can have some that commute and some that don't).  Maybe it's to show
that the context can have more than one patch?

> >+-- | 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".

I'll amend this

> >+-- | 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.

And this.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090423/48156d42/attachment.pgp>


More information about the darcs-users mailing list