darcs

Patch 1856 tentativelyMergePatches: rename anonpend... (and 10 more)

Title tentativelyMergePatches: rename anonpend... (and 10 more)
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-07-29.16:33:52 by bf, last changed 2019-08-06.11:16:08 by ganesh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2019-07-29.16:33:51 text/x-darcs-patch
patch-preview.txt bf, 2019-08-05.21:22:13 text/x-darcs-patch
return-eqcheck-instead-of-repository-from-requirenounrecordedchanges.dpatch bf, 2019-08-05.21:22:13 application/x-darcs-patch
tentativelymergepatches_-rename-anonpend-to-anonpw-for-consistency.dpatch bf, 2019-07-29.16:33:51 application/x-darcs-patch
unnamed bf, 2019-07-29.16:33:51 text/plain
unnamed bf, 2019-08-05.21:22:13 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21004 (view) Author: bf Date: 2019-07-29.16:33:51
Random selection of cleanups and minor refactors.

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

patch ad404366d882ccc8e318bc0a35fbd20098f53f81
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul  9 18:32:54 CEST 2019
  * tentativelyMergePatches: rename anonpend to anonpw for consistency

patch a0bdfd080a49e6f0f7595af1ba209b331d2d6653
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul 14 13:45:34 CEST 2019
  * clean up imports in two modules

patch aa9698db53abd5b29ad88a71c9380677b05f9399
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul 16 19:24:54 CEST 2019
  * add 2 TODO items with V3INTEGRATION tag to Darcs.Util.Graph

patch 432e3b65104266ed2fa8346b704ac9ee32256dab
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 17 14:44:05 CEST 2019
  * replace Darcs.Patch.Witnesses.Sealed.Stepped with Data.Functor.Compose

patch 62c407b37a318fab5813084ada1d67de83f7bfb4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 17 17:36:08 CEST 2019
  * harness: add explanation to an XXX comment

patch bfff45457a20c3108180071d2c50fa54806a8c9f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 18 21:18:05 CEST 2019
  * cleanup in an import of D.P.TouchesFiles

patch b7142aa256af354195ac06a05128f2837dbb63c5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 19 09:48:34 CEST 2019
  * amend: make options local to definition of DarcsCommand
  
  This saves lots of rather verbose type signatures. We did this a while ago
  for most of the other commands.

patch 6293f0deb71753f493ddf5dfb9ab9d8f20bb2bde
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Jul 20 14:25:30 CEST 2019
  * cleanup layout of doActualRecord

patch 0c7bcfcc9762ed26218a0d0561ea04984f3a83a4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul 28 21:24:27 CEST 2019
  * simplify D.P.V1.Prim and D.P.V2.Prim
  
  GeneralizedNewtypeDeriving can nowadays derive more instances: Apply,
  CommuteNoConflicts, and Show. Also remove unused extension
  StandaloneDeriving, simplify instance ReadPatch for V1 (similar to the one
  for V2), and uniformly rename FileNameFormat parameter to 'fmt'.

patch 9ac4e8f19cc5e62b9a7aedad956e6b97a6558519
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul 28 21:38:06 CEST 2019
  * rename data constructors of FileNameFormat

patch c79cc65abec9b81565c804acade2a9b355f023fd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Jul 13 11:25:34 CEST 2019
  * some code cleanups for unsuspendCmd
  
  This factors out the check that there are no unrecorded changes into a local
  procedure that returns a Repository with new witnesses if successful, or
  else dies. We also pass the command name to 'unsuspendCmd' to improve the
  error message for the reify command and remove a number of pattern type
  signatures that are no longer needed.
Attachments
msg21026 (view) Author: ganesh Date: 2019-08-05.13:55:48
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Jul  9 18:32:54 CEST 2019
>   * tentativelyMergePatches: rename anonpend to anonpw for 
>     consistency
OK

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Jul 14 13:45:34 CEST 2019
>   * clean up imports in two modules

OK

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Jul 16 19:24:54 CEST 2019
>   * add 2 TODO items with V3INTEGRATION tag to Darcs.Util.Graph

OK 


> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Wed Jul 17 14:44:05 CEST 2019
>   * replace Darcs.Patch.Witnesses.Sealed.Stepped with 
>     Data.Functor.Compose

OK

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Wed Jul 17 17:36:08 CEST 2019
>   * harness: add explanation to an XXX comment

OK 

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Jul 18 21:18:05 CEST 2019
>   * cleanup in an import of D.P.TouchesFiles

OK 

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Fri Jul 19 09:48:34 CEST 2019
>   * amend: make options local to definition of DarcsCommand

OK (though I'd like to find a better solution, I think we've 
discussed it before)

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sat Jul 20 14:25:30 CEST 2019
>   * cleanup layout of doActualRecord

OK (I assume no code changes)

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Jul 28 21:24:27 CEST 2019
>   * simplify D.P.V1.Prim and D.P.V2.Prim
>   
>   GeneralizedNewtypeDeriving can nowadays derive more instances: 
>   Apply,
>   CommuteNoConflicts, and Show. Also remove unused extension
>   StandaloneDeriving, simplify instance ReadPatch for V1 (similar 
>   to the one
>   for V2), and uniformly rename FileNameFormat parameter to 'fmt'.

It's not clear from the code which way the classes are now being 
derived. Could this be clarified with DerivingStrategies?

https://gitlab.haskell.org/ghc/ghc/wikis/commentary/compiler/derivin
g-strategies

If we really are switching to GeneralizedNewtypeDeriving that would 
be a behaviour change from the existing code for Show. The old 
instance is roughly the same as "deriving stock". I think that's 
probably better for debugging purposes.

> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Jul 28 21:38:06 CEST 2019
>   * rename data constructors of FileNameFormat

OK
msg21030 (view) Author: ganesh Date: 2019-08-05.14:27:21
> patch c79cc65abec9b81565c804acade2a9b355f023fd
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sat Jul 13 11:25:34 CEST 2019
>   * some code cleanups for unsuspendCmd
>   
>   This factors out the check that there are no unrecorded changes 
>   into a local procedure that returns a Repository with
>   new witnesses if successful, or else dies.

OK. I'm not sure which style is superior, using IsEq <- ... to prove 
wR = wU for the rest of the do-block, or returning a new repository 
value, but I don't feel too strongly.

>   We also pass the command name to 'unsuspendCmd' to improve the
>   error message for the reify command and remove a number of
>   pattern type signatures that are no longer needed.

Looks good.
msg21033 (view) Author: bf Date: 2019-08-05.17:04:49
>>   * some code cleanups for unsuspendCmd
>>   
>>   This factors out the check that there are no unrecorded changes 
>>   into a local procedure that returns a Repository with
>>   new witnesses if successful, or else dies.
> 
> OK. I'm not sure which style is superior, using IsEq <- ... to prove 
> wR = wU for the rest of the do-block, or returning a new repository 
> value, but I don't feel too strongly.

Good point.

Using IsEq <- ... in a do block is perfectly fine with me. It just
didn't occur to me that we could do that without having to re-introduce
the ugly pattern type signatures. I just tried it and found they are not
needed at all. And since the procedure in question doesn't modify the
repo in any way, I think returning an EqCheck here is actually better.
Will send a patch.
msg21035 (view) Author: bf Date: 2019-08-05.21:22:13
Following up on review.

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

patch 62d9b1dbc38e9f9fe7d7e6d2884e9833b00b019c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Aug  5 19:10:36 CEST 2019
  * return EqCheck instead of Repository from requireNoUnrecordedChanges
Attachments
msg21036 (view) Author: bf Date: 2019-08-05.22:16:12
>>   * amend: make options local to definition of DarcsCommand
> 
> OK (though I'd like to find a better solution, I think we've 
> discussed it before)

Yes we did. I still think this is currently our best option.

>>   * cleanup layout of doActualRecord
> 
> OK (I assume no code changes)

Correct.

>>   * simplify D.P.V1.Prim and D.P.V2.Prim
>>   
>>   GeneralizedNewtypeDeriving can nowadays derive more instances: 
>>   Apply,
>>   CommuteNoConflicts, and Show. Also remove unused extension
>>   StandaloneDeriving, simplify instance ReadPatch for V1 (similar 
>>   to the one
>>   for V2), and uniformly rename FileNameFormat parameter to 'fmt'.
> 
> It's not clear from the code which way the classes are now being 
> derived. Could this be clarified with DerivingStrategies?

Interesting point. I didn't think of the ambiguity and merely assumed
that 'deriving Show' would produce the same effect whether or not
GeneralizedNewtypeDeriving is in effect or not.

> If we really are switching to GeneralizedNewtypeDeriving that would 
> be a behaviour change from the existing code for Show. The old 
> instance is roughly the same as "deriving stock". I think that's 
> probably better for debugging purposes.

I agree. However,
https://downloads.haskell.org/~ghc/8.6.5/docs/html/users_guide/glasgow_exts.html#a-more-precise-specification
explicitly says that Read, Show, Typeable, or Data are always derived
using stock strategy, unless this is explicitly overridden.

For the other two classes, 'deriving newtype' is what happens by default
and again this is what we want.

We could clarify the strategy using DerivingStrategies. However, this
also places a burden on the developer: they have to think about which
strategy to use in which case. I think relying on the default is safer:
it was carefully chosen to be semantically compatible, so that merely
switching GeneralizedNewtypeDeriving on or off does not change the behavior.
msg21039 (view) Author: ganesh Date: 2019-08-06.09:40:05
On 05/08/2019 23:16, Ben Franksen wrote:

> We could clarify the strategy using DerivingStrategies. However, this
> also places a burden on the developer: they have to think about which
> strategy to use in which case. I think relying on the default is safer:
> it was carefully chosen to be semantically compatible, so that merely
> switching GeneralizedNewtypeDeriving on or off does not change the behavior.

My view is that this is something the developer should be explicitly
aware of, and that would also make reading the code easier. I'd rather
not have to remember the exact GHC rules to know what's going on.

That said, if you remember that stock beats everything in cases where it
would matter, then the rules do lead to warnings on other ambiguities:

https://downloads.haskell.org/~ghc/8.6.5/docs/html/users_guide/glasgow_exts.html#default-deriving-strategy

So it's not too important to me.
History
Date User Action Args
2019-07-29 16:33:52bfcreate
2019-07-29 16:34:47bfsetstatus: needs-screening -> needs-review
2019-08-05 13:55:49ganeshsetstatus: needs-review -> review-in-progress
messages: + msg21026
2019-08-05 14:27:22ganeshsetmessages: + msg21030
2019-08-05 17:04:49bfsetmessages: + msg21033
2019-08-05 21:22:13bfsetfiles: + patch-preview.txt, return-eqcheck-instead-of-repository-from-requirenounrecordedchanges.dpatch, unnamed
messages: + msg21035
2019-08-05 22:16:13bfsetmessages: + msg21036
2019-08-06 09:40:05ganeshsetmessages: + msg21039
2019-08-06 09:43:17ganeshsetstatus: review-in-progress -> accepted-pending-tests
2019-08-06 11:16:08ganeshsetstatus: accepted-pending-tests -> accepted