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

Ben Franksen bugs at darcs.net
Sat Jul 13 22:30:38 UTC 2019


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.

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?

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.

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]

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.

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 = ()

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.

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


More information about the darcs-devel mailing list