[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