[darcs-devel] [patch1617] refactor and cleanup D.UI.SelectChanges and D.P.Choices

[Guillaume Hoffmann]

Guillaume Hoffmann <guillaumh at gmail.com> added the comment:

TD;DR: Screened, and Accepted pending a followup patch that would
simplify the new Invert instance (see below).


* removed trash from D.UI.SelectChanges

OK (I would have used the term "commented code" :) )

  * use mixed RL-FL catenation operators in D.UI.SelectChanges
  
  This saves us another bunch of unneeded reverseFL/RL calls.

Yes!!

  * break some overlong lines in askAboutDepends

OK

  * renamed lpPatch to unLabel

Makes sense.

  * D.UI.SelectChanges: move functions definitions closer to their use

OK

  * refactor D.UI.SelectChanges.runSelection

Seems correct.


  * restrict pattern matching on WhichChanges to functions forward,
backward, and reversed

This simplifies many functions. OK.

  * renamed liftR to generalizeR and added a comment

OK

  * fixed layout of InteractiveSelectionContext

OK (also adds a comment)

  * pull calls to unLabel into repr

Good!!

  * slightly improved haddocks for WhichChanges

OK

  * new code layout for Darcs.Patch.TouchesFiles

OK

  * refactor runSelection once more
  
  This patch concentrates all functions that are concerned with either
  pre-filtering the input patch list or post-filtering the selection
result in
  the where clause of runSelection. It turns out that these are all pure
  functions that can be combined using ordinary function composition.
This was
  previously obscured by the use of monadic style in the Reader monad, which
  we no longer need because the PatchSelectionContext is in scope inisde the
  where clause of runSelection.


Seems correct. Just one thing.
In D.Patch.Invert, the following seems better to me:

~~~
    instance Invert p => Invert (p :> p) where
      invert (a :> b) = invert b :> invert a
~~~

  * fixed layout of some type dignatures in D.P.Choices

OK

  * replace patchChoicesLpsSub with labelPatches
  
  The left component of the pair was never used except in patchChoicesLps
  which we now implement more directly.

Good!

  * inlined patchSlot' (it is just state . patchSlot)

Good!


  * removed forward from D.UI.SelectChanges
  
  The code is easier to modify if there is only one function that makes this
  distinction, and not two, so we now use backward throughout.

Good!


  * renamed modChoices to modifyChoices in D.UI.SelectChanges

OK


  * fix documentation of WhichChanges, backward, and reversed in
D.UI.SelectChanges

OK

  * export mkPatchChoices and inline patchChoicesLps, removing the latter

OK (I find the code in Whatsnew obscure anyway).


  * cleanup type signatures in D.UI.SelectChanges

Yes! Death to the useless forall's and types in pattern matching.


  * fixed some comments in D.P.Choices

OK

  * remove instance Label Ord
  
  The instance is not needed and not having it makes it clear that user code
  is not supposed to rely on it. This give greater freedom if we want to
use a
  different representation at some point (e.g. unique IDs).

OK

  * use Int instead of Integer for Labels
  
  Int should be large enough in practice and is a lot faster.

Agreed, OK

  * remove liftLP by inlining it into its single call site

Good!

  * add lots of documentation to D.P.Choices
  
  The module is now fully haddock'd and cleaned up in many ways, but its
  functionality and implementation details have remained untouched. A
few very
  minor refactorings are included. I renamed some of the internal variables
  (mostly in an effort to understand the implementation better, which in
turn
  was necessary to write correct documentation), and also one or two of the
  exported functions to better fit with the rest of the module.
  

Great, I like the new comments.

----------
status: needs-screening -> accepted-pending-tests

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