* David Roundy <[EMAIL PROTECTED]> [081022 16:37]: > On Tue, Oct 21, 2008 at 08:54:17PM +0200, Chistian Kellermann wrote: > > Tue Oct 21 17:49:35 CEST 2008 Chistian Kellermann <[EMAIL PROTECTED]> > > * extend get_sendmail_cmd to search for 'sendmail' pref and SENDMAIL env > > similar to get_author > > > > With this I can store my sendmail command setting in > > _darcs/prefs/defaults or in $EMAIL. For this the get_sendmail_cmd > > function is unfortunately no longer pure, and the apply as well as > > the send commands had to be changed accordingly. > > > > What I don't like about this patch is the code duplication in > > apply.lhs' redirect_output. Maybe someone with a better haskell > > knowledge can help me out here? > > I'll wait on docs for this, as every environment variable we use > should be documented in the section of the manual... which you can > grep for, that talks about such things. This is important for > enabling debugging of darcs. > OK!
> In the meantime, if anyone thinks there is a better name for this > environment variable (or that we shouldn't have it), they'd better > speak up! > > ... Actually, it turns out there are problems with this patch (see > below) so it requires more than just documentation fixes, although it > still requires that. > > > -get_sendmail_cmd :: [DarcsFlag] -> String > > -get_sendmail_cmd (SendmailCmd a:_) = a > > +get_sendmail_cmd :: [DarcsFlag] -> IO(String) > > It's more standard Haskell style to write -> IO String OK! > > > +get_sendmail_cmd (SendmailCmd a:_) = return a > > get_sendmail_cmd (_:flags) = get_sendmail_cmd flags > > hunk ./src/Darcs/Arguments.lhs 1333 > > -get_sendmail_cmd [] = "" > > +get_sendmail_cmd [] = do > > + easy_sendmail <- get_easy_sendmail_cmd > > + case easy_sendmail of > > + Just a -> return a > > + Nothing -> return "" > > + > > +-- | 'get_easy_sendmail' tries to get the sendmail command first from the > > repository preferences, > > +-- then from global preferences, then from environment variables. Returns > > 'Nothing' if it > > +-- could not get it. > > +get_easy_sendmail_cmd :: IO (Maybe String) > > +get_easy_sendmail_cmd = firstJustIO [ get_preflist "sendmail" >>= return . > > firstNotBlank, > > + get_global "sendmail" >>= return > > .firstNotBlank, > > + maybeGetEnv "SENDMAIL" ] > > I would prefer to avoid introducing a separate helper function here. > It's particularly helpful *not* to introduce top-level functions that > are only used once, as it immediately informs the code reader (and > reviewer) the purpose of your function. You can nicely define such > helper functions in a where block (which can be a bit confusing before > you're use to the indentation rules). > > In this case, I wouldn't write a helper at all, but would write > > get_sendmail_cmd [] = do sm <- firstJustIO [ ..., > ... ] > case sm of > Just a -> ... > > > However, there's another issue that I just noticed, which is your use > of get_preflist and get_global. I'd rather not use that function, as > it means that we're adding two more config files, which would also > need to be documented, which would be _darcs/prefs/sendmail and > .darcs/sendmail. Too many ways to configure the same feature is just > confusing, as it means that a user whose sendmail is broken has that > many more places to look for something that's perhaps affecting the > behavior. It's also more to document. So your code will be both > simpler and nicer if you just use maybeGetEnv alone... I have got that idea from the get_author function, maybe that one should get an overhaul as well then? > > > where sendit tempf e@(ExitException ExitSuccess) = > > - do body <- sanitizeFile tempf > > + do scmd <- get_sendmail_cmd opts > > + body <- sanitizeFile tempf > > sendEmail f to "Patch applied" cc scmd body > > throwIO e > > I think a nice way to rewrite these to reduce duplication would be to > change the sanitizeFile function to a sendSanitizedEmail function, > which would do what sanitizeFile does, and call get_sendmail_cmd and > do sendEmail. This would be a net improvement over the existing code. > :) Alright, thanks for your insights, I will come up with a cleaner patch. Christian -- You may use my gpg key for replies: pub 1024D/47F79788 2005/02/02 Christian Kellermann (C-Keen)
pgpzBM5RYnccV.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
