[darcs-devel] [patch736] Style Darcs.SelectChanges (and 2 more)
Owen Stephens
bugs at darcs.net
Tue Mar 6 23:05:23 UTC 2012
Owen Stephens <darcs at owenstephens.co.uk> added the comment:
Hi Andreas,
Thanks for the patches. I've made a few comments inline.
> [Style Darcs.SelectChanges
> Andreas Brandt <andreas.brandt.de at googlemail.com>**20120306184242
> Ignore-this: 40305ee44a317015df8e7869c916c83f
> ] hunk ./src/Darcs/SelectChanges.hs 245
> keysFor = concatMap (map kp)
>
> -- | The function for selecting a patch to amend record. Read at your
own risks.
> -withSelectedPatchFromRepo :: forall p C(r u t). (RepoPatch p, ApplyState
p ~ Tree)
> - => String -> Repository p C(r u t) ->
[DarcsFlag]
> - -> (FORALL(a) (FL (PatchInfoAnd p) :>
PatchInfoAnd p) C(a r) -> IO ()) -> IO ()
> +withSelectedPatchFromRepo ::
> + forall p C(r u t). (RepoPatch p, ApplyState p ~ Tree)
> + => String -- name of calling command (always "amend" as of now)
> + -> Repository p C(r u t)
> + -> [DarcsFlag]
> + -> (FORALL(a) (FL (PatchInfoAnd p) :> PatchInfoAnd p) C(a r) -> IO
())
> + -> IO ()
> withSelectedPatchFromRepo jn repository o job = do
> p_s <- readRepo repository
> hunk ./src/Darcs/SelectChanges.hs 254
> - sp <- wspfr jn (matchAPatchread o)
> - (newset2RL p_s) NilFL
> + sp <- wspfr jn (matchAPatchread o) (newset2RL p_s) NilFL
All looks good up to here.
> case sp of
> Just (FlippedSeal (skipped :> selected')) -> job (skipped :>
selected')
> Nothing -> do putStrLn $ "Cancelling "++jn++" since no patch was
selected."
> hunk ./src/Darcs/SelectChanges.hs 259
>
> --- | This ensures that the selected patch commutes freely with the
skipped patches, including pending
> --- and also that the skipped sequences has an ending context that
matches the recorded state, z,
> --- of the repository.
> +-- | This ensures that the selected patch commutes freely with the
skipped
> +-- patches, including pending and also that the skipped sequences has an
> +-- ending context that matches the recorded state, z, of the repository.
Maybe this could take this opertunity to name this function something
better. I
just sat for a while wondering what on earth wspfr could stand for :-) I
know
it's part of the development tips wiki page, but I'm not a fan of this
naming
convention...
> wspfr :: (RepoPatch p, ApplyState p ~ Tree)
> hunk ./src/Darcs/SelectChanges.hs 263
> - => String -> (FORALL(a b) (PatchInfoAnd p) C(a b) -> Bool)
> - -> RL (PatchInfoAnd p) C(x y) -> FL (PatchInfoAnd p) C(y u)
> + => String
> + -> (FORALL(a b) (PatchInfoAnd p) C(a b) -> Bool)
> + -> RL (PatchInfoAnd p) C(x y)
> + -> FL (PatchInfoAnd p) C(y u)
> -> IO (Maybe (FlippedSeal (FL (PatchInfoAnd p) :> (PatchInfoAnd
p)) C(u)))
> wspfr _ _ NilRL _ = return Nothing
> wspfr jn matches (p:<:pps) skipped
> [Resolve issue2145: darcs amend-record now can be navigated with 'j'
> Andreas Brandt <andreas.brandt.de at googlemail.com>**20120306195825
> Ignore-this: d906e2588f8e7e18e391c2336d61340c
> ] hunk ./src/Darcs/SelectChanges.hs 269
> -> FL (PatchInfoAnd p) C(y u)
> -> IO (Maybe (FlippedSeal (FL (PatchInfoAnd p) :> (PatchInfoAnd
p)) C(u)))
> wspfr _ _ NilRL _ = return Nothing
> -wspfr jn matches (p:<:pps) skipped
> +wspfr jn matches remaining@(p:<:pps) skipped
> | not $ matches p = wspfr jn matches pps (p:>:skipped)
> | otherwise =
> case commuteFLorComplain (p :> skipped) of
> hunk ./src/Darcs/SelectChanges.hs 283
> [[ KeyPress 'y' (jn++" this patch")
> , KeyPress 'n' ("don't "++jn++" it")
> , KeyPress 'k' "back up to previous patch"
> + , KeyPress 'j' "skip to next patch"
> ]]
> advanced_options =
> [[ KeyPress 'v' "view this patch in full"
> hunk ./src/Darcs/SelectChanges.hs 303
> 'k' -> case skipped of
> NilFL -> repeat_this
> (prev :>: skipped') -> wspfr jn matches (prev :<: p
:<: pps) skipped'
> + 'j' -> case remaining of
> + NilRL -> repeat_this
> + (prev :<: remaining') -> wspfr jn matches remaining'
(prev :>: skipped)
You've already matched against a non-NilRL value, so you don't need to
match it
again (the first wspfr case catches the NilRL case).
> 'v' -> printPatch p >> repeat_this
> 'p' -> printPatchPager p >> repeat_this
> 'x' -> do putDocLn $ prefix " " $ summary p
> [Tidy up wspfr
> Andreas Brandt <andreas.brandt.de at googlemail.com>**20120306203717
> Ignore-this: 36ef9444431a9c0850526c4256f1561f
> Some style improvements and renaming variables to CamelCase
> ] hunk ./src/Darcs/SelectChanges.hs 256
> p_s <- readRepo repository
> sp <- wspfr jn (matchAPatchread o) (newset2RL p_s) NilFL
> case sp of
> - Just (FlippedSeal (skipped :> selected')) -> job (skipped :>
selected')
> - Nothing -> do putStrLn $ "Cancelling "++jn++" since no patch was
selected."
> + Just (FlippedSeal (skipped :> selected')) -> job (skipped :>
selected')
Why not use an @-pattern here, too?
> + Nothing ->
> + putStrLn $ "Cancelling " ++ jn ++ " since no patch was
selected."
>
> -- | This ensures that the selected patch commutes freely with the
skipped
> -- patches, including pending and also that the skipped sequences has an
> hunk ./src/Darcs/SelectChanges.hs 279
> wspfr jn matches pps (p:>:skipped)
> Right (skipped' :> p') -> do
> printFriendly [] p
> - let repeat_this = wspfr jn matches (p:<:pps) skipped
> - basic_options =
> - [[ KeyPress 'y' (jn++" this patch")
> - , KeyPress 'n' ("don't "++jn++" it")
> - , KeyPress 'k' "back up to previous patch"
> - , KeyPress 'j' "skip to next patch"
> - ]]
> - advanced_options =
> - [[ KeyPress 'v' "view this patch in full"
> - , KeyPress 'p' "view this patch in full with pager"
> - , KeyPress 'x' "view a summary of this patch"
> - , KeyPress 'q' ("cancel "++jn)
> - ]]
> - let prompt' = "Shall I "++jn++" this patch?"
> - yorn <- promptChar $ PromptConfig { pPrompt = prompt'
> - , pBasicCharacters = keysFor
basic_options
> - , pAdvancedCharacters = keysFor
advanced_options
> - , pDefault = Just 'n'
> - , pHelp = "?h" }
> + yorn <- promptChar $
> + PromptConfig { pPrompt = prompt'
> + , pBasicCharacters = keysFor basicOptions
> + , pAdvancedCharacters = keysFor
advancedOptions
> + , pDefault = Just 'n'
> + , pHelp = "?h" }
> case yorn of
> 'y' -> return $ Just $ flipSeal $ skipped' :> p'
> 'n' -> wspfr jn matches pps (p:>:skipped)
> hunk ./src/Darcs/SelectChanges.hs 289
> 'k' -> case skipped of
> - NilFL -> repeat_this
> - (prev :>: skipped') -> wspfr jn matches (prev :<: p
:<: pps) skipped'
> + NilFL -> repeat_this
> + (prev :>: skipped') ->
> + wspfr jn matches (prev :<: remaining) skipped'
> 'j' -> case remaining of
> hunk ./src/Darcs/SelectChanges.hs 293
> - NilRL -> repeat_this
> - (prev :<: remaining') -> wspfr jn matches remaining'
(prev :>: skipped)
> + NilRL -> repeat_this
> + (prev :<: remaining') ->
> + wspfr jn matches remaining' (prev :>: skipped)
> 'v' -> printPatch p >> repeat_this
> 'p' -> printPatchPager p >> repeat_this
> 'x' -> do putDocLn $ prefix " " $ summary p
> hunk ./src/Darcs/SelectChanges.hs 300
> repeat_this
> - 'q' -> do putStrLn $ jn_cap++" cancelled."
> + 'q' -> do putStrLn $ jn_cap ++ " cancelled."
> exitWith $ ExitSuccess
> hunk ./src/Darcs/SelectChanges.hs 302
> - _ -> do putStrLn $ helpFor jn basic_options advanced_options
> + _ -> do putStrLn $ helpFor jn basicOptions advancedOptions
> repeat_this
> hunk ./src/Darcs/SelectChanges.hs 304
> - where jn_cap = (toUpper $ head jn) : tail jn
> + where repeat_this = wspfr jn matches (p:<:pps) skipped
> + prompt' = "Shall I " ++ jn ++ " this patch?"
> + jn_cap = (toUpper $ head jn) : tail jn
> + basicOptions =
> + [[ KeyPress 'y' (jn ++ " this patch")
> + , KeyPress 'n' ("don't " ++ jn ++ " it")
> + , KeyPress 'k' "back up to previous patch"
> + , KeyPress 'j' "skip to next patch"
> + ]]
> + advancedOptions =
> + [[ KeyPress 'v' "view this patch in full"
> + , KeyPress 'p' "view this patch in full with pager"
> + , KeyPress 'x' "view a summary of this patch"
> + , KeyPress 'q' ("cancel " ++ jn)
> + ]]
>
> -- After selecting with a splitter, the results may not be canonical
> canonizeAfterSplitter :: (FL p :> FL p) C(x y) -> Reader
(PatchSelectionContext p) ((FL p :> FL p) C(x y))
> replace ./src/Darcs/SelectChanges.hs [A-Za-z_0-9] jn_cap jnCapital
> replace ./src/Darcs/SelectChanges.hs [A-Za-z_0-9] p_s patchSet
> replace ./src/Darcs/SelectChanges.hs [A-Za-z_0-9] repeat_this repeatThis
You could also replace basic_options since it's used as an identifier in
lastQuestion, too.
Otherwise, great - thanks!
--
Owen.
__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch736>
__________________________________
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20120306/b919b781/attachment-0001.html>
More information about the darcs-devel
mailing list