Hi! Lots of little nitpicky comments. Apologies, but these are purely stylistic. Hopefully somebody else can look at the content?
On Tue, Oct 21, 2008 at 21:47:59 +0200, Chistian Kellermann wrote:
> Tue Oct 21 21:44:53 CEST 2008 Chistian Kellermann <[EMAIL PROTECTED]>
> * half-resolve issue628: reuse the long comment code from Darcs.record,
> export therefore prompt_long_comment
In the future, it would be helpful if you could keep the short patch
name <= 72 characters :-)
> Remaining issues:
> - Haddock documentation is missing (will follow in a separate patch)
You probably want to keep this in the bundle description and not in the
patch description
> - Lockfile issue: When doing a darcs record or darcs tag with this
> and the user hits CTL-C the lock is not removed. How should I do
> this? I haven't found the place in Darcs.Record where this happens
I'll leave this for somebody else to comment
> - adjustment of Testcases.
Should this necessary?
half-resolve issue628: reuse the long comment code from Darcs.record, export
therefore prompt_long_comment
----------------------------------------------------------------------------------------------------------
> \label{record}
> \begin{code}
> {-# OPTIONS_GHC -cpp -fglasgow-exts #-}
> -module Darcs.Commands.Record ( record, commit, get_date, get_log,
> file_exists ) where
> +module Darcs.Commands.Record ( record, commit, get_date, get_log,
> file_exists, prompt_long_comment ) where
> import Control.Exception ( handleJust, Exception( ExitException ) )
> import Control.Monad ( filterM, when )
> import System.IO ( hGetContents, stdin )
> hunk ./src/Darcs/Commands/Record.lhs 286
> (PriorPatchName p, []) -> return p
> (NoPatchName, []) -> prompt_patchname
> True
> -- round 2
> - append_info f firstname
> + append_info f firstname chs
> when (EditLongComment `elem` fs) $ do edit_file f
> return ()
> (name, thelog, _) <- read_long_comment f firstname
> hunk ./src/Darcs/Commands/Record.lhs 294
> return (name, thelog, Nothing)
> gl (EditLongComment:_) =
> case patchname_specified of
> - FlagPatchName p -> actually_get_log p
> - PriorPatchName p -> actually_get_log p
> - NoPatchName -> prompt_patchname True >>=
> actually_get_log
> + FlagPatchName p -> actually_get_log p default_log
> make_log chs
> + PriorPatchName p -> actually_get_log p default_log
> make_log chs
> + NoPatchName -> do p <- prompt_patchname True
> + actually_get_log p default_log
> make_log chs
> gl (NoEditLongComment:_) =
> case patchname_specified of
> FlagPatchName p
> hunk ./src/Darcs/Commands/Record.lhs 309
> return (p, [], Nothing)
> gl (PromptLongComment:fs) =
> case patchname_specified of
> - FlagPatchName p -> prompt_long_comment p -- record (or
> amend) -m
> + FlagPatchName p -> prompt_long_comment p default_log
> make_log chs -- record (or amend) -m
> _ -> gl fs
> gl (_:fs) = gl fs
> gl [] = case patchname_specified of
> hunk ./src/Darcs/Commands/Record.lhs 314
> FlagPatchName p -> return (p, [], Nothing) -- record
> (or amend) -m
> - PriorPatchName "" -> prompt_patchname True >>=
> prompt_long_comment
> + PriorPatchName "" -> do p <- prompt_patchname True
> + prompt_long_comment p
> default_log make_log chs
> PriorPatchName p -> return (p, default_log, Nothing)
> hunk ./src/Darcs/Commands/Record.lhs 317
> - NoPatchName -> prompt_patchname True >>=
> prompt_long_comment
> + NoPatchName -> do p <- prompt_patchname True
> + prompt_long_comment p default_log
> make_log chs
> prompt_patchname retry =
> do n <- askUser "What is the patch name? "
> if n == "" || take 4 n == "TAG "
> hunk ./src/Darcs/Commands/Record.lhs 325
> then if retry then prompt_patchname retry
> else fail "Bad patch name!"
> else return n
> - prompt_long_comment oldname =
> - do yorn <- promptYorn "Do you want to add a long comment?"
> - if yorn == 'y' then actually_get_log oldname
> - else return (oldname, [], Nothing)
> - actually_get_log p = do logf <- make_log
> - writeBinFile logf $ unlines $ p :
> default_log
> - append_info logf p
> - edit_file logf
> - read_long_comment logf p
> - read_long_comment :: FilePathLike p => p -> String -> IO (String,
> [String], Maybe p)
> - read_long_comment f oldname =
> - do t <- (lines.filter (/='\r')) `fmap` readBinFile f
> - case t of [] -> return (oldname, [], Just f)
> - (n:ls) -> return (n, takeWhile
> - (not.(eod `isPrefixOf`)) ls,
> - Just f)
> - append_info f oldname =
> - do fc <- readBinFile f
> - appendToFile f $ \h ->
> - do case fc of
> - _ | null (lines fc) -> hPutStrLn h oldname
> - | last fc /= '\n' -> hPutStrLn h ""
> - | otherwise -> return ()
> - hPutDocLn h $ text eod
> - $$ text ""
> - $$ wrap_text 75
> - ("Place the long patch description above the
> "++
> - eod++
> - " marker. The first line of this file "++
> - "will be the patch name.")
> - $$ text ""
> - $$ text "This patch contains the following
> changes:"
> - $$ text ""
> - $$ summary (fromPrims chs :: Patch)
>
> eod :: String
> eod = "***END OF DESCRIPTION***"
> hunk ./src/Darcs/Commands/Record.lhs 328
> +prompt_long_comment :: String -> [String] -> IO String -> FL Prim -> IO
> (String, [String], Maybe String)
> +prompt_long_comment oldname default_log make_log chs =
> + do yorn <- promptYorn "Do you want to add a long comment?"
> + if yorn == 'y' then actually_get_log oldname default_log make_log chs
> + else return (oldname, [], Nothing)
One trick to make patches easier to review is to first create a
refactoring patch that doesn't change any functionality, and then send a
little patch which actually changes something. This isn't a fixed rule
or anything, just a sort of idea on how to do things in reader-friendly
way :-)
Another thought is that maybe the functions below could be private
to the prompt_long_comment
Finally, since we are exporting this function, we should be using
camelCase for its name.
> +actually_get_log :: String -> [String] -> IO String -> FL Prim ->
> + IO (String, [String], Maybe String)
> +actually_get_log p default_log make_log chs =
> + do logf <- make_log
> + writeBinFile logf $ unlines $ p : default_log
> + append_info logf p chs
> + edit_file logf
> + read_long_comment logf p
The indentation here is a bit odd-looking. I think you may be using
tabs in your text-editor... (spaces please!) Likewise for the rest
of the functions below!
> hunk ./src/Darcs/Commands/Tag.lhs 96
> +In accordance with the record command the user gets the chance to add a long
> comment
> +using the same interface as darcs record.
Cool! Thanks for updating the user's manual along the way.
> +\begin{code}
> +get_long_comment :: [DarcsFlag] -> String -> IO [String]
> +get_long_comment (NoEditLongComment:_) _ = return []
> +get_long_comment (_:flags) oldname = get_long_comment flags oldname
> +get_long_comment [] oldname = do
> + let make_log = world_readable_temp "darcs-tag"
> + (_, comment, _) <- prompt_long_comment oldname
> [] make_log NilFL
> + return comment
Maybe by default, this shouldn't ask for long comments. Only if we
detect the PromptLongComment flag should we ask for one?
On a more general note, Jason was saying a while ago on #darcs that we
need a more principled approach to dealing with flag defaults. Here is
another potential use case.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgp7QI4RkPw1a.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
