[darcs-devel] [patch1454] add wrapper type around 'Named' (and 10 more)

[Ganesh Sittampalam]

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

On 22/03/2016 16:57, Guillaume Hoffmann wrote:

> * stop Convert using Wrapped.patchcontents: OK (patch name misleading?
>    seems to me that  this patch is only a consequence of the
>    "add wrapper type" patch)

Oops, yes. This patch no longer seems to have anything to do with the
patchcontents function :-) Originally it did, but I suspect that changed
as I refactored my patch sequence and I didn't notice.

> * Introduce RebaseP to replace Rebasing type:
> 
>    Now RepoType gets a lot of new stuff. It gets a parameter of type
>    RebaseType, which is new, and defined as:
>    
>    data RebaseType = IsRebase | NoRebase
>    
>    We have new classes IsRebaseType and IsRepoType.
>    
>    Restriction (IsRepoType rt) *seems* to be added to all function
>    signatures that involve Patch-related types.

Sort of - it's added to all functions that need to explicitly switch at
runtime on the repository type (rebase or no-rebase). That's not
necessarily all functions that use patches/repositories.

>    I don't really get the complexity of IsRepoType, SRepoType and
>    SRebaseType .. is all of this necessary?

I don't know of an alternative. TBH it's fairly simple type-level stuff,
but I appreciate it does add extra complexity.

>    The RepoType module could use more comments. Intuitively I get that
>    all of this enables us to specify at the type level whether some
>    operation can be done on repositories with rebase patches or not,
>    but the module needs more explanation.
> 
>    As a consequence (but I don't get why), this removes the MaybeInternal
>    class restriction to lots of functions of, eg, Darcs.Patch.Match.

Essentially because we can now explicitly pattern-match on RebaseP
instead of needing to use the MaybeInternal class to detect the rebase
container patch. That's one of the wins of this refactoring, it reduces
the hackery required for rebase.

>    The patch also adds the RebaseP constructor to WrappedNamed.
> 
>    Heavy part: new functions in the same module:
>    
>    * fmapFL_WrappedNamed
>        :: (FL p wA wB -> FL q wA wB)
>           -> (RebaseTypeOf rt :~~: 'IsRebase -> p :~: q)
>           -> WrappedNamed rt p wA wB
>           -> WrappedNamed rt q wA wB
>     fmapFL_WrappedNamed f _ (NormalP n) = NormalP (fmapFL_Named f n)
>     fmapFL_WrappedNamed _ whenRebase (RebaseP n s) =
>       case whenRebase ReflRebaseType of
>         ReflPatch -> RebaseP n s
> 
>     I see from the implementation that pattern-matching in the "case"
>     can fail. Why not explicitely use error?

Where exactly should I use error in this implementation?

There is a situation at the use sites where there's a function that can
never be called (with a non-bottom parameter) and I'd like to use an
empty case expression to express that, but can't yet because of
library/GHC versions.

Cheers,

Ganesh

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