On Wed, Jun 16, 2010 at 17:20:26 +0000, Adolfo Builes wrote: > Tue Jun 15 09:44:02 COT 2010 [email protected] > * Resolve issue 1176: caches interfere with --remote-repo flag
I already reviewed this, so I'll skip it. > Wed Jun 16 11:23:30 COT 2010 [email protected] > * Removes duplicated entries after any modification is done to the cache Almost there. Just one more change (to the haddock) Removes duplicated entries after any modification is done to the cache ---------------------------------------------------------------------- > +-- | @modifyCache repository@ function@ modifies the cache of > +-- @repository@ with @funct...@. After the modification is done the > +-- duplicated entries are removed and then the cache is sorted with > +-- local < http < ssh There are three problems with the haddock that I can see: - You have a syntax error in the top line (note that I never ask people to amend haddock syntax errors, A. because I'm so grateful they even bother to write them and B. because it's more efficient for me to just fix it, point it out and be done with it) - You are repeating the documentation for 'Darcs.Repository.Cache.compareByLocality' when you could just as easily link to it. Duplication is the root of all evil. Imagine we one day decide to change 'compareByLocality' and we (inevitably) forget to update this haddock. It's the kind of thing that's subtle, we'll never notice it for years until somebody does something wrong... - Your comment reveals too much about the implementation of the function. Your job is not to tell me what the code says, but I as a user of the function can expect from it. In particular, X happens, then Y and Z is not entirely appropriate. The subtlety here is that (f . sort) is not the same as (sort . f). Maybe we should think about this on IRC Can you fix these? I think once you address the two key issues above, I can just apply what you have and maybe do some minor cleaning up > - where dr = DarcsRepository pristine newCache > - newCache = ( \ (Ca c) -> Ca $ sortBy compareByLocality c) $ f cache > + where dr = DarcsRepository pristine $ nubAndSort $ f cache > + nubAndSort (Ca c) = Ca $ sortBy compareByLocality $ nub c This seems fine. I think the way I would written it was something like the below (again with the Functor idea), but it could just be my taste, which is flawed :-) dr = DarcsRepository pristine $ cmap (nub . sortBy compareByLocality . f) cache cmap f (Ca c) = Ca (f c) But you should probably ignore my comment above -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
pgpRmfb6G2vGu.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
