[darcs-devel] darcs patch: Added the ability to add long comments to tags. By de...

[David Roundy]

On Tue, Sep 13, 2005 at 01:56:10PM +0100, Tom Counsell wrote:
> On 13 Sep 2005, at 13:15, David Roundy wrote:
> > I'd really prefer it to be implemented somewhat differently (see
> > below).
> 
> I didn't expect I'd get it right on first try, so thank you for the  
> feedback!

You're welcome!

> >>+\begin{code}
> >>+get_log :: [DarcsFlag] -> String  -> IO (String, [String])
> >>+get_log opts oldname = gl opts
> >>
> >[...]
> >
> >I really dislike like the fact that we're duplicating this code.  It
> >should be pretty straightforward (although it means learning a bit more
> >Haskell) to add get_log to the export list of Record.lhs, and then add
> >to Tag.lhs an import statement "import Record ( get_log )".  Then you
> >won't need to import nearly as much to Tag.  I'm working under the
> >assumption here that you haven't had to modify get_log... if that's not
> >the case, then I'd like to hear what you needed to do to it.
> 
> I needed to modify it to:
> 1) Default to not prompting the user
> 2) Eliminate the reference to chs (patches?)

The second you could handle by converting the get_log in Record to be

get_log :: [DarcsFlag] -> String -> Doc -> IO (String, [String])

where the Doc would be interpereted as "interesting information to pass to
the user".  So in Record, the caller would pass

(patch_summary (join_patches chs))

as the Doc argument, but in Tag you'd probably pass emptyDoc (or whatever
it's called).  Or you could pass some other sort of information that might
be handy (like a list of the changes included in the new tag, for instance,
or just the number of patches included).

The first is a bit tricky to handle beautifully.  One possibility (a bit
ugly) is to make get_log *require* that a relevant flag be passed (dying
with "bug" if a flag isn't found), and then when calling it explicitely
pass the default at the end of the list.  So you'd have to explicitely look
for PromptLongComment as well as EditLongComment and NoEditLongComment.
Then in Record you'd pass (opts++[PromptLongComment]) to set the default,
and in Tag you'd pass (opts++[NoEditLongComment]).  As I say, this is a bit
ugly, since it depends on the fact that we accept the "first" flag that we
find, which is weird.  I prefer using code like (EditLongComment `elem`
opts), which fails with this scheme for setting the default.  Another plan
would be to explicitely pass in the default behavior as a separate flag:

get_log :: [DarcsFlag] -> DarcsFlag -> String -> Doc -> IO (String, [String])
get_log opts defaultopt oldname info
  | Pipe `elem` opts = ...
  | EditLongComment `elem` opts = ...
  | ...
  | otherwise = case defaultopt of
                NoEditLongComment -> ...
                PromptLongComment -> ...
                _ -> impossible
  where ... (here you define functions for NoEditLongComment etc, so we
             don't need duplicated code above)

If you want help on this, let me know.  I prefer giving advice and
criticising your code, since my experience has been that that leads to
better, less buggy code than I'd produce on my own, but it also takes more
time and effort, and this rewriting would require considerably more
haskell-learning than you've done so far.  (But if you *want* to learn
haskell, you'll probably learn to write "nice" haskell--at least by my
definition--much faster than you'd learn by yourself.)

> Anyway, I'll see what I can do for an improved version (with test).

Great!
-- 
David Roundy
http://www.darcs.net