On Tue, Sep 2, 2008 at 12:54 PM, <[EMAIL PROTECTED]> wrote:
My attempt at reviewing....
> Tue Sep 2 03:04:05 CEST 2008 [EMAIL PROTECTED]
> * Change type of subdir parameter in Cache/HashedIO functions from String to
> HashedDir.
>
> This refactor should make calling the Cache and HashedIO functions safer: you
> should be no longer able to swap hash and subdir accidentally in the call
> site,
> or mistype the subdirectory name.
That sounds positive.
But, I wonder about this hunk:
hunk ./src/Darcs/Repository/HashedIO.lhs 70
-}
data HashDir r p = HashDir { permissions :: !r, cache :: !Cache,
- options :: ![DarcsFlag], rootHash :: !String,
- hashDir :: !String }
+ options :: ![DarcsFlag], rootHash :: !String }
type HashedIO r p = StateT (HashDir r p) IO
Why did you remove the hashDir from the record? Why did you change
hashDir :: HashedDir?
> Tue Sep 2 20:57:45 CEST 2008 [EMAIL PROTECTED]
> * Refactor Cache's handling of hashed paths. No functional change.
>
> Factored out the filepath building to a single place. This also led to
> folding
> the explicit pattern matches on writability into a predicate, since the other
> components of a CacheLoc are no longer useful in the function bodies.
This seems fine to me.
>
> Tue Sep 2 21:35:59 CEST 2008 [EMAIL PROTECTED]
> * Use qualified imports and shorter names on Cache functions. No functional
> change.
I like qualified imports just fine. I think Data.ByteString sets a
good example here. We don't do it much in darcs currently, but I
think I like this change.
That's my review. I would accept all the above patches modulo the
hashDir change.
Thanks,
Jason
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
