[darcs-devel] darcs patch: add make_changelog, a tool to auto-gener... (and 10 more)

[Ian Lynagh]

On Wed, May 11, 2005 at 11:50:18PM +0200, Tomasz Zielonka wrote:
> I needed to fix a bug I made in GNUmakefile in darcs-stable - this
> resurrected the conflict in unstable. Ian, if you have any problems with
> this or you simply don't like it, just ignore these patches and tell me,
> and I will record them again. I would even prefer this myself, but I
> don't know if you've already applied these patches to your repos.

I haven't applied any of them, so rerecording sounds good if you wanted
to do that anyway  :-)

I've made a few comments/suggestions below.

> +        (`mapM` fnames) $ \fn -> do
> +            cs <- readFile fn
> +            case parse entryFile fn cs of
> +                Left err -> fail (show err)
> +                Right x -> return x

I find this very peculiar, and the following much easier to understand:

       let parse fn = do
           cs <- readFile fn
           case parse entryFile fn cs of
               Left err -> fail (show err)
               Right x -> return x
       mapM parse fnames

Is it just me?

> +    history <- liftM (reverse . takeWhile (not . (apply_matcher matchTag_1_0_2)) . concat) (read_repo ".")

I prefer keeping lines under 80 chars (where possible).

Also, couldn't this give the wrong answer if patches are reordered?
Probably not a problem for the darcs-stable repo itself, I guess.

> +    hPutStrLn stderr $ "unmatched entries (upcoming?):"

That $'s unnecessary.

> +              else do
> +                return (Just (patterns', descr))

Here you can just write:

    else return (Just (patterns', descr))

(no "do")

> +-- FIXME: format date in rfc-2822, don't use show for ClockTime
> +show_pi_date :: PatchInfo -> String
> +show_pi_date pinfo = show (toClockTime (pi_date pinfo))

System.Time has:

    formatCalendarTime :: TimeLocale -> String -> CalendarTime -> String
    
    formats calendar times using local conventions and a formatting string.

    The formatting string is that understood by the ISO C strftime()
    function.

There's also:

Prelude> System.Locale.rfc822DateFormat
"%a, %_d %b %Y %H:%M:%S %Z"

I'd guess you want to pass System.Locale.defaultTimeLocale for the
TimeLocale.

> +matchTag_1_0_2 :: Matcher
> +matchTag_1_0_2 = match_pattern (PatternMatch "exact \"TAG 1.0.2\"")

Shouldn't this ("1.0.2") be an argument to the program, and the function
renamed accordingly?

> +entry :: CharParser st ChangeLogEntry
> +entry = do
> +    patterns <- many1 $ do
> +        try (string "match:")
> +        pattern <- restOfLine
> +        newline
> +        return $! (match_pattern (PatternMatch pattern))

Is this $! important?


Thanks
Ian