The last of these three patches is the best thing since
sliced bread. Please can we screen it? Please? :-)

Something has gone a bit wrong with the tests on my
machine (with or without this patch) but I'm reasonably
sure it doesn't cause any regressions.

3 patches for repository darcs-unstable@darcs.net:screened:

patch ddfc58366c4a698c72cf6761fef4e3f69aba76fc
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 16:23:25 BST 2019
  * get rid of unused ShowDictRecord

patch 93ddb175af5fc40bed7f9cefd3d6eb3a33808393
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 16:35:47 BST 2019
  * get rid of unused exports/functions from D.P.W.Show

patch e6abaccc8645490509c53ce3e7ce4f6b60568d14
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 17:40:34 BST 2019
  * use QuantifiedConstraints for Show1/Show2
  
  This allows us to remove a lot of boilerplate instances
  including many hand-written Show instances.
  
  Note that where we have something like Show2 (PrimOf p),
  we have to write (prim ~ PrimOf p, Show2 prim):
  https://gitlab.haskell.org/ghc/ghc/issues/14860
  
  It does mean we now require GHC 8.6, and also some of
  the instances now require UndecidableInstances.

Attachments

• get-rid-of-unused-showdictrecord.dpatch
• patch-preview.txt
• unnamed

Go ahead. The savings in boilerplate code are impressive! I don't mind 
the occasional UndecidableInstances, I had to add one or two myself.

Screening them now.

The only reason I am hesitating to accept this now is that we may want to check if we can depend on ghc-6.8 for the next release of darcs.

That was the reason I didn't immediately screen it, apologies
if I didn't make it clear enough.

QuantifiedConstraints are fundamental to it, so I think
we'd have to bring back the boilerplate to support any earlier
GHCs.

> That was the reason I didn't immediately screen it, apologies
> if I didn't make it clear enough.

Okay. Looks like we should obliterate it as soon as possible before we
push (any more?) patches that depend on it. I think rebasing this patch
is a lot easier than rolling it back. BTW, hub.darcs.net now allows to
obliterate patches, see their FAQ.

Well, maybe we can think about whether depending on 8.6 is actually 
a  problem? When do you anticipate making a release?

8.6 was released in September 2018 so it's not completely bleeding 
edge. On the other hand it looks like it won't be in Debian stable 
for a couple of years, as it missed the last release last month:
https://wiki.debian.org/DebianReleases

We could ask on darcs-users once we have a vague idea of when
we would release.

> Well, maybe we can think about whether depending on 8.6 is actually a
> problem? When do you anticipate making a release?
> 
> 8.6 was released in September 2018 so it's not completely bleeding 
> edge. On the other hand it looks like it won't be in Debian stable 
> for a couple of years, as it missed the last release last month: 
> https://wiki.debian.org/DebianReleases

Not supporting the ghc in debian stable would be a shame. I really like
this patch but I would like to release darcs-3.0 sooner than the next
debian stable.

The largest remaining stumbling block is currently repo conversion to
darcs-3. This is absolutely essential for any real-world usage. Other
issues like nailing down the on-disk format aren't /that/ important: the
difficult part of conversion are the semantic differences, whereas an
optimization (like avoiding to store most of the prim patch IDs) could
be done later and in a fully compatible way, similar to the upgrade to
the hashed repo format.

Therefore I think we should really obliterate this patch from screened,
even though it pains me to say that.

I don't mind rolling it back instead of trying to orchestrate
an obliteration - presumably the tag you've made also depends on it
now?

My original plan, before I realised how good QuantifiedConstraints 
were, was to use the 'constraints' package more heavily, so at
least I can go to that state instead in the "rollback". And maybe
some cunning trick will turn up for the instances after all.
Unfortunately supporting GHC < 8.6 also rules out the use of 
DerivingVia.

> I don't mind rolling it back instead of trying to orchestrate
> an obliteration - presumably the tag you've made also depends on it
> now?

Yes. I personally find an obliterate cleaner. Not much orchestration
needed, I would just do it. Shall I?

> My original plan, before I realised how good QuantifiedConstraints 
> were, was to use the 'constraints' package more heavily, so at
> least I can go to that state instead in the "rollback".

I don't know much about the constraints package, but I am worried about
adding more dependencies and "constraints" sounds like one of those
heavy packages of which we would need just a tiny part?

The Show boilerplate doesn't hurt /that/ much, besides the instances
aren't used a lot, most of the unit test code and error messages use
displayPatch when something fails which I find a lot more readable
anyway. I guess we can just leave the instances in there and (mostly)
ignore them. For now.

> And maybe
> some cunning trick will turn up for the instances after all.
> Unfortunately supporting GHC < 8.6 also rules out the use of 
> DerivingVia.

Yea, this is sad. I have used it intensively in my current
Sequence/Equality branch. Not in screened, though.

>> My original plan, before I realised how good QuantifiedConstraints 
>> were, was to use the 'constraints' package more heavily, so at
>> least I can go to that state instead in the "rollback".
> 
> I don't know much about the constraints package, but I am worried about
> adding more dependencies and "constraints" sounds like one of those
> heavy packages of which we would need just a tiny part?

To prove it, here is a patch that removes this dependency. Open to
discussion, of course.

1 patch for repository http://darcs.net/screened:

patch 1d5a700e83472ad81025abec8d9608b9c5c9b405
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Aug 21 18:11:59 CEST 2019
  * replace dependency on constraints package with trivial copy of Dict data type

Attachments

• patch-preview.txt
• replace-dependency-on-constraints-package-with-trivial-copy-of-dict-data-type.dpatch
• unnamed

We can certainly inline bits of other packages we want to use,
but 'constraints' isn't a big dependency and I think using a 
standard type also helps to communicate that we are using a standard 
concept to other people reading our code. I don't care that much 
though.

> We can certainly inline bits of other packages we want to use,
> but 'constraints' isn't a big dependency

Not as big as some of the others, true. I just counted: 6 modules
exporting 190 symbols. Of which we need one.

I remember one guy complaing on @darcs-users about having to download &
compile what feels half of hackage... I feel that too, whenever I switch
to a new compiler version.

What other packages did you have in mind?

> and I think using a 
> standard type also helps to communicate that we are using a standard 
> concept to other people reading our code.

What other people? ;-) But I get your point. Communicating intent is
good, even of there is (currently) not much of an audience. (That's why
I added a comment to the code stating that this is 1:1 exactly the data
type from the constraints package.)

> I don't care that much though.

Me neither.

I would like to accept the first two patches. Can you make a separate 
bundle?

Here you go, I'll mark the old bundle dead.

2 patches for repository /home/ganesh/darcs/screened-temp:

patch ddfc58366c4a698c72cf6761fef4e3f69aba76fc
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 16:23:25 BST 2019
  * get rid of unused ShowDictRecord

patch 93ddb175af5fc40bed7f9cefd3d6eb3a33808393
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 16:35:47 BST 2019
  * get rid of unused exports/functions from D.P.W.Show

Attachments

• get-rid-of-unused-showdictrecord.dpatch
• patch-preview.txt
• unnamed

>> We can certainly inline bits of other packages we want to use,
>> but 'constraints' isn't a big dependency

> Not as big as some of the others, true. I just counted: 6 modules
> exporting 190 symbols. Of which we need one.

> I remember one guy complaing on @darcs-users about having to download &
> compile what feels half of hackage... I feel that too, whenever I switch
> to a new compiler version.

> What other packages did you have in mind?

Nothing in particular, but now I look, 'hashable', 'filepath',
'data-ordlist', 'html' (The latter dependency is in fact a bit dubious
as we're actually using it for XML output), 'split' (actually that might
really be not used - but if we were using just one function from it
the same argument would apply).

We can distinguish 'constraints' from these others because there's really
only one way to write that type, so I would accept that it's at one extreme
of the spectrum.

>>> We can certainly inline bits of other packages we want to use,
>>> but 'constraints' isn't a big dependency
> 
>> Not as big as some of the others, true. I just counted: 6 modules
>> exporting 190 symbols. Of which we need one.
> 
>> I remember one guy complaing on @darcs-users about having to download &
>> compile what feels half of hackage... I feel that too, whenever I switch
>> to a new compiler version.
> 
>> What other packages did you have in mind?
> 
> Nothing in particular, but now I look, 'hashable',

Hm, yes. This one's also quite dubious.

It is only used for the patience diff. I may be wrong but it looks as if
they manually implemented a hash table there, with linear lookup in case
of a hash collision. This should be refactored to either use
Data.HashMap (another dependency, though I'd say justified if it gives
us better performance here) or else use a plain Map with ByteString keys
(if Data.HashMap doesn't perform better).

BTW, the diff algorithms are severely under-tested. We should add
properties and test them!

> 'filepath',

From that library we use at least

</>
<.>
combine
dropFileName
dropTrailingPathSeparator
isAbsolute
isRelative
isValid
joinPath
normalise
pathSeparator
splitDirectories
takeDirectory
takeFileName

Obviously justified by the number of symbols alone. It also takes care
of doing the right thing for each OS. I know, I know, we still have to
be very careful whether to use the native or the Posix module (more
often than not it's the Posix module). Still better than doing all this
stuff ourselves. (And in some places we still do; yet another TODO item.)

> 'data-ordlist'

We import nubSort, isect, minus from that one. These functions are not
very complicated but also not completely trivial to get right, so I'd
say the dependency is justified.

> 'html' (The latter dependency is in fact a bit dubious
> as we're actually using it for XML output)

Yup. Should use a proper XML lib here. Generating correct XML is also
not trivial (text must be properly encoded), so I'd say a dependency is
justified. From a quick search it looks as if
http://hackage.haskell.org/package/X could fit our needs. I may give it
a try.

, 'split' (actually that might
> really be not used - but if we were using just one function from it
> the same argument would apply).

We don't use it and I will remove the dependency (of darcs-test on split).

> We can distinguish 'constraints' from these others because there's really
> only one way to write that type, so I would accept that it's at one extreme
> of the spectrum.

I also don't like the name of the data type. All the libraries above
export functions that by their name alone give a good indication of what
they will do. Not so with 'Dict'. You need to know about the semantics
of classes in terms of its translation to passing a dictionary of
methods in order to "intuitively" grok what 'Dict' is about. And someone
who comes from Python will most certainly be confused...

All fine (without the one requiring ghc-8.6).