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.
|