darcs

Patch 1866 clean up the Matchable mess (and 1 more)

Title clean up the Matchable mess (and 1 more)
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-08-11.16:08:24 by bf, last changed 2019-08-13.00:56:08 by ganesh.

Files
File name Status Uploaded Type Edit Remove
clean-up-the-matchable-mess.dpatch dead bf, 2019-08-11.16:08:23 application/x-darcs-patch
clean-up-the-matchable-mess.dpatch bf, 2019-08-12.15:29:00 application/x-darcs-patch
patch-preview.txt dead bf, 2019-08-11.16:08:23 text/x-darcs-patch
patch-preview.txt bf, 2019-08-12.15:29:00 text/x-darcs-patch
unnamed dead bf, 2019-08-11.16:08:23 text/plain
unnamed bf, 2019-08-12.15:29:00 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21076 (view) Author: bf Date: 2019-08-11.16:08:23
If this is desired for easier review I can spend some effort to split the
first patch up into smaller parts.

2 patches for repository http://darcs.net/screened:

patch 6718fadd517b50d7a8224fb20b5f6bd2144e2bdb
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 19 13:53:42 CEST 2019
  * clean up the Matchable mess
  
  This is a large refactor. Matchable is no longer a class but a constraint
  synonym. This measn we don't have to define instances and can therefore
  defined it in Darcs.Patch.Match. Darcs.Patch.Matchable has been deleted.
  
  MatchFun, Matcher, and PatchSetMatch no longer get the patch type or repo
  type as parameter. Instead, MatchFun is now a data type that contains the
  polymorphic match function (constrained to work only on Matchable patches).
  
  Matchable p now also requires Ident p and PatchId p ~ PatchInfo, so we have
  access to the PatchInfo (using ident). The goal here was to generalize
  matching functions from PatchInfoAnd to any Matchable patch type. Since some
  of the matching functions work on PatchSets, which have PatchInfoAnd
  hard-coded, we add a new constraint synonym for RepoPatches p such that
  PatchInfoAnd rt p becomes Matchable.
  
  Darcs.UI.SelectPatches no longer has PatchInfoAnd in any of its type
  signatures.

patch 4f5a602dcbed83b18d2de05b67c91d6a246ff9a5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul 29 18:04:27 CEST 2019
  * eliminate Darcs.Patch.Dummy
  
  This is possible since Darcs.Patch.Match no longer needs it. The few
  remaining ocurrences were easily refactored to avoid its use.
Attachments
msg21078 (view) Author: ganesh Date: 2019-08-11.19:39:35
> If this is desired for easier review I can spend some effort to 
> split the first patch up into smaller parts.

I've reviewed it as it stands, it was quite easy as it was really 
only a few easy-to-follow changes in the type signatures (unless I 
missed some significant code changes?).

Perhaps it could have been clearer still with the different changes 
broken apart, but certainly not worth the effort now.

Go head and push it (either with or without amending - the patch 
description typo is unimportant and the comment typo can be fixed in 
a followup if easier).

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 19 13:53:42 CEST 2019
>   * clean up the Matchable mess
>   
>   This is a large refactor. Matchable is no longer a class but a 
>   constraint
>   synonym. This measn we don't have to define instances and can 
>   therefore
>   defined it in Darcs.Patch.Match. Darcs.Patch.Matchable has been 
>   deleted.

Typo in the patch description ('measn').

>   MatchFun, Matcher, and PatchSetMatch no longer get the patch 
>   type or repo
>   type as parameter. Instead, MatchFun is now a data type that 
>   contains the
>   polymorphic match function (constrained to work only on 
>   Matchable patches).

This seems like a nice change as it gets rid of annoying type 
parameters, and the existence of Dummy to instantiate it was an 
indication that something wasn't quite right.

On the other hand it will stop us writing matchers specific to a 
single patch type in future, but that's very hypothetical.

>   Matchable p now also requires Ident p and PatchId p ~ PatchInfo, 
>   so we have
>   access to the PatchInfo (using ident). The goal here was to 
>   generalize
>   matching functions from PatchInfoAnd to any Matchable patch 
>   type. Since some
>   of the matching functions work on PatchSets, which have 
>   PatchInfoAnd
>   hard-coded, we add a new constraint synonym for RepoPatches p 
>   such that
>   PatchInfoAnd rt p becomes Matchable.
>   
>   Darcs.UI.SelectPatches no longer has PatchInfoAnd in any of its 
>   type
>   signatures.

> hunk ./src/Darcs/Patch/Match.hs 137
[..]> -- | Constraint for a patch type @p@ that ensures 
@'PatchInoAnd' rt p@

Typo in the doc comment ('PatchInoAnd')

> hunk ./src/Darcs/UI/SelectChanges.hs 1008
> -askAboutDepends :: (IsRepoType rt, RepoPatch p, ApplyState p ~ 
Tree)
> +askAboutDepends :: ( IsRepoType rt, RepoPatch p, ApplyState p ~ 
Tree )

Not a big deal, but this seems like a gratutious and slightly 
negative whitespace change. We don't normally put spaces around one-
line constraints here, do we?

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Mon Jul 29 18:04:27 CEST 2019
>   * eliminate Darcs.Patch.Dummy


Nice.
msg21086 (view) Author: bf Date: 2019-08-12.15:11:51
I think I will fix the problems you found, then amend, re-send and push.
msg21087 (view) Author: bf Date: 2019-08-12.15:29:00
Fixed & rebased bundle.

2 patches for repository http://darcs.net/screened:

patch 3ca1278615e363ae85cbee04c81f356dcba61b53
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Aug 12 17:24:53 CEST 2019
  * clean up the Matchable mess
  
  This is a large refactor. Matchable is no longer a class but a constraint
  synonym. This means we don't have to define instances and can therefore
  defined it in Darcs.Patch.Match. Darcs.Patch.Matchable has been deleted.
  
  MatchFun, Matcher, and PatchSetMatch no longer get the patch type or repo
  type as parameter. Instead, MatchFun is now a data type that contains the
  polymorphic match function (constrained to work only on Matchable patches).
  
  Matchable p now also requires Ident p and PatchId p ~ PatchInfo, so we have
  access to the PatchInfo (using ident). The goal here was to generalize
  matching functions from PatchInfoAnd to any Matchable patch type. Since some
  of the matching functions work on PatchSets, which have PatchInfoAnd
  hard-coded, we add a new constraint synonym for RepoPatches p such that
  PatchInfoAnd rt p becomes Matchable.
  
  Darcs.UI.SelectPatches no longer has PatchInfoAnd in any of its type
  signatures.

patch 1020fcd35876eaa0e2f9a8e6cbe6c988ff13abd9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Aug 12 17:26:20 CEST 2019
  * eliminate Darcs.Patch.Dummy
  
  This is possible since Darcs.Patch.Match no longer needs it. The few
  remaining ocurrences were easily refactored to avoid its use.
Attachments
History
Date User Action Args
2019-08-11 16:08:24bfcreate
2019-08-11 19:39:35ganeshsetmessages: + msg21078
2019-08-12 15:11:51bfsetmessages: + msg21086
2019-08-12 15:29:01bfsetfiles: + patch-preview.txt, clean-up-the-matchable-mess.dpatch, unnamed
messages: + msg21087
2019-08-12 15:30:23bfsetstatus: needs-screening -> needs-review
2019-08-12 22:53:19ganeshsetstatus: needs-review -> accepted-pending-tests
2019-08-13 00:56:08ganeshsetstatus: accepted-pending-tests -> accepted