[darcs-users] darcs patch: half-resolve issue628: reuse the long comment code fro...

Eric Kow kowey at darcs.net
Tue Oct 21 20:50:41 UTC 2008


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 <Christian.Kellermann at nefkom.net>
>   * 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20081021/27b31c31/attachment.pgp 


More information about the darcs-users mailing list