On Wed, Jun 16, 2010 at 17:20:26 +0000, Adolfo Builes wrote:
> Tue Jun 15 09:44:02 COT 2010 builes.adolfo@googlemail.com
> * 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 builes.adolfo@googlemail.com
> * 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 @function@. 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
|