darcs

Patch 1943 get rid of the PrimPatchBase superclass for Summary

Title get rid of the PrimPatchBase superclass for Summary
Superseder Nosy List ganesh
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-09-27.12:25:42 by ganesh, last changed 2020-01-30.17:41:30 by bfrk.

Files
File name Status Uploaded Type Edit Remove
get-rid-of-the-primpatchbase-superclass-for-summary.dpatch ganesh, 2019-09-27.12:25:41 application/x-darcs-patch
patch-preview.txt ganesh, 2019-09-27.12:25:41 text/x-darcs-patch
unnamed ganesh, 2019-09-27.12:25:41 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21635 (view) Author: ganesh Date: 2019-09-27.12:25:41
This is a simplification broken out of the rebase-uses-prim
work.

It does mean some more Foo (PrimOf p) constraints but none
are undecidable and I think it's well worth it.

I'll leave it for a little bit before screening.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 2b5f014d8a8442296c7effe547d8620f27038148
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Sep 27 13:29:45 BST 2019
  * get rid of the PrimPatchBase superclass for Summary
  
  It sort of makes sense in theory, especially given that
  the type of conflictedEffect mentions PrimOf, but in
  practice it infects instances in unpleasant ways.
Attachments
msg21639 (view) Author: bfrk Date: 2019-09-27.15:55:40
> This is a simplification broken out of the rebase-uses-prim
> work.

I don't get it. How does this simplify anything? I see a clear increase
in complexity of constraints, overall. Also, I don't understand what's
so unpleasant about the occasional extra PrimPatch or PrimPatchBase
constraint (3 alltogether in your patch).
msg21640 (view) Author: ganesh Date: 2019-09-27.16:11:04
> I don't get it. How does this simplify anything? I see a clear increase
> in complexity of constraints, overall. Also, I don't understand what's
> so unpleasant about the occasional extra PrimPatch or PrimPatchBase
> constraint (3 alltogether in your patch).

I was actually provoked to do it by some nasty cascading constraints
that the PrimPatchBase superclass entailed when doing the rebase
refactoring, so to some extent it was about preventing future complexity
as well. But I can't remember the exact details now and can't quickly
reproduce them. I expect I'll come across them again as I continue
cleaning up that refactoring.

As a general principle I think this is a bad superclass though. Why
should we need all the stuff in PrimPatch (PrimOf p) just to be able to
implement Summary? It obscures the modularity of our code when we have
huge constraints on instances.
msg21651 (view) Author: bfrk Date: 2019-09-27.23:39:20
>> I don't get it. How does this simplify anything? I see a clear increase
>> in complexity of constraints, overall. Also, I don't understand what's
>> so unpleasant about the occasional extra PrimPatch or PrimPatchBase
>> constraint (3 alltogether in your patch).
> 
> I was actually provoked to do it by some nasty cascading constraints
> that the PrimPatchBase superclass entailed when doing the rebase
> refactoring, so to some extent it was about preventing future complexity
> as well. But I can't remember the exact details now and can't quickly
> reproduce them. I expect I'll come across them again as I continue
> cleaning up that refactoring.

Okay, fine. If it helps prevent getting into trouble there, go ahead.

> As a general principle I think this is a bad superclass though. Why
> should we need all the stuff in PrimPatch (PrimOf p) just to be able to
> implement Summary? It obscures the modularity of our code when we have
> huge constraints on instances.

True. The superclass couples things more tightly than necessary.

In general I guess this is the price we pay for being able to shorten
constraints. I mean, how much of the functionality of PrimPatch or
RepoPatch do you really need in each single case? Usually only a small
fraction. Still lots of our code is riddled with PrimPatch and RepoPatch
constraints, simply because it is more convenient than to write down all
the classes we actually need. It also makes the constraint more robust
against changes because you don't have to adapt lots of constraints when
you shift functionality from one place to another.

So it's a trade-off. I usually give up when I reach 4 or 5 constraints
and just use one of the two "big hammers" ;-))

(And I see now why you were tempted to split PrimOf out of PrimPatchBase
as that would mean that PrimPatchBase no longer is a "logically
justified" superclass.)
msg21652 (view) Author: ganesh Date: 2019-09-28.06:41:07
> So it's a trade-off. I usually give up when I reach 4 or 5 constraints
> and just use one of the two "big hammers" ;-))

Yes, same here. I guess the real point I meant to make is that the
superclass is bad because it *forces* us to have huge constraints on
instances. For a specific instances it's a purely local trade-off and we
can switch between the two approaches quite easily.

I ran into exactly this after patch1945 (removing a PrimPatch constraint
from a data constructor), where I threw in a couple of RepoPatches and
then discovered in patch1911 that they were also annoying (partly
because of me having thrown Annotate (PrimOf p) into RepoPatch - that
may want revisiting/moving down to PrimPatch). But it was easy to
substitute them at each use site so it wasn't a big deal.
msg21766 (view) Author: bfrk Date: 2020-01-30.17:41:30
It doesn't hurt and doesn't break anything; and it's arguably a bit clearer this way.
History
Date User Action Args
2019-09-27 12:25:42ganeshcreate
2019-09-27 15:55:40bfrksetmessages: + msg21639
2019-09-27 16:11:04ganeshsetmessages: + msg21640
2019-09-27 23:39:20bfrksetmessages: + msg21651
2019-09-28 06:41:07ganeshsetmessages: + msg21652
2019-09-28 06:53:21ganeshsetstatus: needs-screening -> needs-review
2020-01-30 17:41:30bfrksetstatus: needs-review -> accepted
messages: + msg21766