[darcs-devel] [patch484] split out IsHunk from Effect (and 23 more)

Ganesh Sittampalam ganesh at earth.li
Sun Jan 2 18:36:38 UTC 2011


On Sun, 2 Jan 2011, Eric Kow wrote:

> All look good.  My only complaint is that we're missing license
> boilerplate on a lot of the new modules we're creating.

Good point.

This is kind of annoying, because it involves figuring out a reasonably 
copyright line even though most of our other copyright lines are way out 
of date. Any idea how other projects approach this problem?

> Here's the meat of this patch.  Note the disappearance of the
> isHunk default implementation which ...
>
>> hunk ./src/Darcs/Patch/Named.lhs 44
>> +instance IsHunk (Named p) where
>> +    isHunk _ = Nothing
>
>> +instance IsHunk (PatchInfoAnd p) where
>> +    isHunk _ = Nothing
>
> Shows up here and here.  Any reason not to have a default implementation
> anymore?  Guess it's safer to avoid accidental fall-through.

I'm on another mission to remove defaults :-) Sometimes they are the right 
thing to do - e.g. MyEq where you only need to define one member - but I 
think in cases like this accidental fall-through is bad. For example if I 
was making a new patch type is it ok to leave it out? I think the answer 
for this particular method is generally no.

More widely, I've found quite a few examples where things are defaulted, 
and then for many instances the methods aren't actually ever used. I think 
I've got some refactorings of Conflict and Apply pending (perhaps not yet 
submitted) where this is the case and by teasing them apart into multiple 
type classes you can then remove some instances.

> I asked on IRC if automatic unused-function detection would be useful
> for us...  Turns out that's exactly what Ganesh did to make some of the
> refactor code.

Unfortuately the tool I wrote to do it requires various hacks so isn't 
very easy to use generally - both the darcs source code need to be changed 
to make it parseable, and haskell-src-exts needs some changes, some of 
which aren't generally correct but work ok in the darcs case. I will try 
to get it cleaned up for hackage at some point though.

> abstract MarkedUpFile etc over PatchInfo
> ----------------------------------------
> Not entirely sure where this is going, but I'm imagining that it's for
> the on-going work in making conflict marking report the patches
> involved.

Actually, not that. I needed to do it to untangle some dependencies (I 
forget the details), but I felt justified in doing so because it makes to 
logical sense; it proves the independence of the concept of marking up a 
file from the actual data you mark it up with.

> (also maybe an interesting case where you might still want a hunk move patch
> because it says what you mean, even if it makes the patch less concise because
> you end up having to edit the hunks anyway)

Yep, I'd have done that if we had hunk move. Ideally a combination of 
indentation, selective token replace and hunk move could have expressed 
the whole thing :-)

> FileHunk, eh?

It's a file patch that's a hunk. Made sense to me, anyway :-)

Ganesh


More information about the darcs-devel mailing list