[darcs-devel] [patch2155] rebase cleanups

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

>>   * remove an unnecessary type application
> OK, assuming it compiles with all our supporter compilers.

This is checked by the github CI which I trigger regularly, usually
before I send a patch bundle.

> Minor nitpick: I wouldn't have removed the forall in the type definition
> of toRebaseChanges as these often turn out to be needed again in the
> future.

But you have to admit that the end result is less cluttered now, and
thus easier to read.

> Also the indentation of the reformatted signature isn't
> resistant to changes in the name of the function.

I merely followed the convention we use almost everywhere else. I agree
that this convention is not ideal for the reason you stated and I am
open to a change here. For type signatures that don't fit on a single
line you seem to prefer

fun
  :: (Constraints)
  => x
  -> y

I could adopt that style. Are you using an auto-formatter? I have used
hindent for a long time, but it has lots of bugs and often the
formatting is not quite what I want so I have to amend the result
manually. I have just now installed brittany and it seems to work a lot
more reliably and does indeed format type signatures in that way.

>>   * remove instance Commute WrappedNamed
> 
>> -- | The opposite of sealing is to duplicate a single witness. This is for
>> -- situations where a patch-like type is expected (i.e. one with two witnesses)
>> -- but you have only one. Naturally, any concrete value must have both witnesses
>> -- agreeing.
> 
> I disagree that this is the opposite of sealing, so I would prefer to
> remove that sentence.

Well, 'Sealed (Dup p wX)' is isomorphic to 'p wX':

fromSealedDup :: Sealed (Dup p wX) -> p wX
fromSealedDup (Sealed (Dup x)) = x

toSealedDup :: p wX -> Sealed (Dup p wX)
toSealedDup x = Sealed (Dup x)

does typecheck, and the two functions are obviously inverses.

So Dup is the opposite of Sealed in the same sense in that
pretty-printing is the opposite of parsing.

I should perhaps elaborate the haddocks to make that clearer.

>>       writeTentativeInventory (repoCache repo) compr (PatchSet ts (unsafeCoerceP ps))
> 
> I think this is a new unsafeCoerce - do you know why it's needed?

Not really. I don't think the new code does anything fundamentally
different wrt the witnesses. The error message says "‘wX0’ is
untouchable" and I have only a vague idea what that means.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2155>
__________________________________