[darcs-devel] [patch2166] cleaning up Arbitrary instances
Ben Franksen
bugs at darcs.net
Wed May 5 10:32:38 UTC 2021
Ben Franksen <ben.franksen at online.de> added the comment:
>> instance Arbitrary (Sealed2 (Prim :> Prim)) where
>
> whereas in D.T.P.A.PrimV1 we have:
>
>> instance Arbitrary (Sealed2 (Prim1 :> Prim1)) where
>> arbitrary = mapSeal2 wsPatch <$> arbitraryPair
>
> which gets used for a completely different set of tests.
>
> It would be much better if these instances were all built in a
> compositional way, with each type constructor like Sealed2, :>, etc,
> just having a single instance that builds on the instances for its
> parameters. That way it's easy to look at a type and work backwards to
> see how its instance will behave, and also to add new instances for new
> things without fear of overlap.
But that is obviously not possible if the pair instances are specialized
for different patch types. I understand that this specialization comes
from the desire to control the distribution of patch pairs. This is why
we have aPrimPair etc. So your Pair type is used to distinguish these
specially generated pairs from generically generated pairs which use a
threaded repo state. Correct?
> Essentially I'm trying to untangle the mess bit by bit. V1Gen is maybe
> the simplest example of this as it's very local to RepoPatchV1.
> Introducing that means that instead of carefully crafting instances like
> the above, we can make the specific tests in D.T.P.RepoPatchV1 that want
> those instances always work via the V1Gen wrapper. I found the specific
> tests by changing the instances to use V1Gen and then working through
> the compiler errors: but now that it's done, we can be confident of
> adding instances that don't mention V1Gen without affecting those tests.
Okay. So we have a better isolation of the messyness now. This is
progress, sure, but I think we can do better. See below.
> Similarly the introduction of classes like ArbitraryS2 means that
> instead of having instances like Arbitrary (Sealed2 Prim), Arbitrary
> (Sealed2 (Prim :> Prim)) etc, we can have a single instance ArbitraryS2
> p => Arbitrary (Sealed2 p). With just that change, we still have
> ArbitraryS2 Prim, ArbitraryS2 (Prim :> Prim) but then further
> refactorings along similar lines can make those better too.
Okay, makes sense.
>> I am trying to understand the big picture...
>
> I guess there isn't a clear big picture that describes my end goals.
The reason I want to talk about the big picture is that I think a lot of
the complications are just technical debt. Take the specialized
generators for RepoPatchV1: do they add any value? I very much doubt it.
My best guess is that when RepoPatchV2 was added to the test suite they
were left in there more or less unchanged due to a mixture of fear (what
if it breaks things?) and laziness (its just ugly test code anyway, why
bother?).
This is nothing you can fix with pure refactors since of course there
are semantic differences. I think most of them are irrelevant. The only
relevant difference I am aware of is that between MergeableSequence and
PatchTree: for RepoPatchV1 we cannot use the former and must the latter
because otherwise many properties fail. So we need to find a way to test
RepoPatchV1 with generators that merely swap MergeableSequence with
PatchTree but otherwise use the same properties and instances as for the
other RepoPatch types.
None of that means your patches are inappropriate. Go ahead and screen
them. But I would like to enourage you to make much more radical cuts,
especially with regard to the legacy RepoPatchV1 generators. Your
refactors should make this easier now.
__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2166>
__________________________________
More information about the darcs-devel
mailing list