This is nice and minimal. Now let's shave it down some more! On Fri, Oct 24, 2008 at 10:18:02 +0200, Christian Kellermann wrote: > Now let's start the nitpicking :D
ok! :-)
resolve issue628: reuse the long comment code from Darcs.record
---------------------------------------------------------------
> +import Data.List ( isPrefixOf )
David: I confirm that this is in GHC 6.6.
> +import Darcs.Lock ( world_readable_temp )
(as used by record and amend record... the clutter left behind when
I cancel these commands is a bit of a nuisance, but then again, that
is more a feature than a bug, so I won't complain)
> - tentativelyAddPatch repository opts $ n2pia $ adddeps mypatch deps
> - finalizeRepositoryChanges repository
> - when (CheckPoint `elem` opts) $ write_recorded_checkpoint repository
> myinfo
> - putStrLn $ "Finished tagging patch '"++name++"'"
Other reviewers: this appears to be indentation-only.
> -\end{code}
> -Each tagged version has a version name.
> -\begin{code}
Note that you've lost this bit of user manual
> -get_patchname :: [DarcsFlag] -> IO String
> -get_patchname (PatchName n:_) = return $ "TAG "++n
> -get_patchname (_:flags) = get_patchname flags
> -get_patchname [] = do
> - n <- askUser "What is the version name? "
> - if n == "" then get_patchname []
> - else return $ "TAG "++n
Unless I am mistaken, by using Darcs.Record.get_log, we now no longer
ask "What is the version name?" but "What is the patch name?"
> + where get_name_log :: [DarcsFlag] -> [String] -> IO (String, [String])
This is going to sound like the opposite of what I suggested last
time, is there a reason why get_name_log is in a where block and not a
top-level function? It's not always easy to get a feel for when to do
one or the other, so it'll be good see your thoughts on it. I was
mainly asking because not moving it may allow you make an even more
minimal patch, but you have had some design-thinking in there...
If you want to put it in a where block, maybe we can get rid of some
arguments, like the [DarcsFlag] one and just use the one from the
surrounding function.
> + get_name_log o [] = do (name, comment, _) <- get_log o Nothing
> (world_readable_temp "darcs-tag") NilFL
> + return (add_tag name, comment)
> + get_name_log o a = do let name = add_tag $ unwords a
> + (comment_name , comment, _) <- get_log
> (add_patch_name o name) Nothing (world_readable_temp "darcs-tag") NilFL
> + putStrLn $ "Comment " ++ comment_name ++
> unlines comment
> + return (name, comment)
Also, is the putStrLn deliberate, or is it left-over debuggin information?
Now it looks to me like you can make this a bit clearer by combining the
two cases
get_name_log o a = do let o2 = if null a then o else (add_patch_name o (unwords
a))
(name, comment, _) <- get_log o2 Nothing
(world_readable_temp "darcs-tag") NilFL
return (add_tag name, comment)
Would something like that work? Or is there some interaction with the
opts that I have missed?
> + add_tag :: String -> String
> + add_tag [] = []
> + add_tag n | "TAG " `isPrefixOf` n = n
> + | otherwise = "TAG " ++ n
Note that this represents a change in behaviour. Now you can no longer
name your tags TAG something (which would be silly to do anyway on the
other hand).
You could just replace the add_tag above by "TAG " ++
> + add_patch_name :: [DarcsFlag] -> String -> [DarcsFlag]
> + add_patch_name o a| has_patch_name o = o
> + | otherwise = [PatchName a] ++ o
So if the user specified a patch name with -m, we ignore the patch
name supplied on the command line args... fair enough, I guess :-)
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
