[darcs-devel] [patch1849] move PrimPatchId into its own module (and 1 more)

Ganesh Sittampalam bugs at darcs.net
Sun Jul 14 10:28:51 UTC 2019


Ganesh Sittampalam <ganesh at earth.li> added the comment:

On 13/07/2019 23:30, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen at online.de> added the comment:
> 
> I have looked at your changes. While in general I appreciate the idea of
> making types abstract, I think including QC as a dependency here goes a
> bit over the top, given that we export data constructors almost
> everywhere. I'm assuming you don't actually plan to invest the effort of
> pulling all Arbitrary instances into darcslib and making all the patch
> types abstract... and even if you did plan on doing that I am not sure I
> would agree that this is a good move.

No - but it is an acknowledgement that sometimes test code best lives in
the modules under test and so darcslib might need to have some testing
dependencies.

I do actually have a bad reason for moving all our test code into
darcslib, which is that I use ghcid for my development workflow and
being able to typecheck all relevant code in a single cabal (new-)repl
invocation makes that a lot easier. But much of our test code is too
horrible to put in darcslib anyway :-)

If we can make the TestOnly nullary type class idea work then I think
that tilts the balance a bit towards putting test code in darcslib.


> If you really believe we need an abstract interface to PrimPatchIds,
> wouldn't it be better to make it powerful enough so that an instance
> Arbitrary can be defined without accessing the internals?

I'm not really sure how to make it powerful enough without just exposing
the ability to construct one from an arbitrary natural. Which we may
conclude we should do.

> You expose the type PrimPatchSerial and functions firstPrimPatchSerial
> and nextPrimPatchSerial. Isn't this, more or less, encoding natural
> numbers a la Peano? There is a reason we use natural numbers: they are
> the canonical implementation of this interface, i.e. all others are
> isomorphic to it. What your interface is telling me is: a PrimPatchId
> behaves as if it consisted of a SHA1, a natural number, and a signedness
> flag.

Agreed. The thing I dislike most about the current code is that callers
can see the negative numbers and might accidentally misuse them. If it
exposed (Natural, Bool) I probably wouldn't have been so motivated to
change it.

> My immediate idea to improve the API was to provide serial numbers as an
> infinite list (one symbol to export, instead of two):
> 
> positivePrimPatchSerials :: [PrimPatchSerial]

That was also my initial idea, until I remembered that lists can be
finite, so the consuming code would need an error case for hitting an
empty list. Or I'd have to write or import a stream type, which seemed
like a bit more overhead/hassle. But given your instinct was the same
way, using Data.Stream seems worthwhile.

> But, looking at how this is actually used, we can do much better:
> 
> positivePrimPatchIds :: SHA1 -> [PrimPatchId]
> 
> that is, we don't have to expose the serial numbers at all. And taking
> this further, can we get rid of the SHA1 as well? I am thinking of a
> "subordinate PatchId" abstraction here: given a parent PatchId, generate
> subordinate PatchIds for children.

This seems like a good idea. The interface could be

normalPrimPatchIds :: PatchInfo -> Stream PrimPatchId

I prefer 'normal'/'inverted' to 'positive'/'negative', but I don't feel
too strongly.


> The fromPrim method is used only to construct Named patches. And it has
> this particular type only because it should work for Prims with and
> without PatchId. What if we pull the PatchId family out of Ident:

> 
> type family PatchId (p :: *->*->*)
> 
> class (SignedId (PatchId p), Eq2 p) => Ident p where
>   ident :: p wX wY -> PatchId p
> 
> This would allow us to define
> 
> class FromPrim p where
>   fromPrim :: PatchId p -> (PrimOf p) wX wY -> p wX wY
> 
> without the requirement of an instance Ident p. Patches without identity
> could define
> > type instance PatchId MyPatchType = ()

This also seems like a good idea which might also help with
loop-breaking the imports for FromPrim.

> Taking up your idea to define container classes, we could then abstract
> subordinate PatchIds using something like
> 
> class SubPatchId f where
>   subPatchId :: PatchId (f p) -> PatchId p
> 
> or perhaps make it return a list. This is unfinished and untested and
> probably doesn't even compile. But I think you get the idea: fix the
> ugly type of fromPrim and find a better abstraction to express that
> PatchIds for RepoPatches are derived from that of their parent
> (container). I am not even sure the name PrimPatchId is appropriate:
> from the outside it appears only as the PatchId of a RepoPatchV3.

The SubPatchId idea seems plausible, though probably for a future
refactoring. And even though I suggested it for testing purposes, I'm
unsure about explicitly enforcing that our Repo patch types have a 'f p'
structure for general code.

I'll have a play with streams and type family PatchId and see if I can
get a reasonable incremental refactoring we can then build on.

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


More information about the darcs-devel mailing list