[darcs-users] darcs patch: Use unless. (and 2 more)

Trent W. Buck trentbuck at gmail.com
Thu Jan 15 10:23:13 UTC 2009


On Thu, Jan 15, 2009 at 09:30:01AM +0000, Eric Kow wrote:
> Hmm... I have some minor comments.  Perhaps resubmit?
> I await your instructions.
> 
> Use unless.
> -----------
> >  ensureDirectories :: WriteableDirectory m => FileName -> m ()
> >  ensureDirectories d = do
> >            isPar <- mDoesDirectoryExist d
> > -          if isPar 
> > -            then return ()
> > -            else ensureDirectories (super_name d) >> (mCreateDirectory d)
> > +          unless isPar $
> > +                 do ensureDirectories $ super_name d
> > +                    mCreateDirectory d
> 
> I might be a little less enthusiastic care-free about making purely
> parenthesis and do-notation vs explicit type refactors (because we can
> flip-flop a bit about which is best for what situation, so it may be
> good just to avoid the churn).

Here I'm basically following the Lisp style of: IF has two branches;
use WHEN or UNLESS if you only have one branch.  Maybe people don't
buy into that philosophy for Darcs?

In any case, I forgot to include "import .. unless" in this patch ^_^;;

> Redundant do.
> -------------
> > +syncSlurpy put s = if unsyncedSlurpySize s > slurp_sync_size
> > +                   then return <<= put s
> > +                   else return s
> 
> return <<= put s could also have been simplified to put s

I'll resubmit.

> Haddockize add_to_list.
> -----------------------
> > +-- | Add a new element to a list, as long as that element is not
> > +-- already in the list.
> >  add_to_list :: Eq a => a -> [a] -> [a]
> >  add_to_list s [] = [s]
> > hunk ./src/Darcs/Repository/Prefs.lhs 384
> > -add_to_list s (s':ss) = if s == s' then (s:ss) else s': add_to_list s ss
> > +add_to_list s (s':ss) | s == s'   = s:ss
> > +                      | otherwise = s': add_to_list s ss
> 
> False advertising,

Well, naughty of me to have dumped the second hunk in there, anyway.

> and also the kind of refactor I would have just
> avoided (churn without appreciable gain in clarity, i.e. one could have
> argued that the previous version (sans excess parens) would have been
> more readable... I don't know which is which, but the fact that it's not
> cut and dry probably means it's wisest to leave it alone)

Sure thing; today I've just been exploring various stylistic refactors
and trying to decide which ones appreciably increase readability.  I
kinda thought this one did because the plain words if, then and else
don't really "stand out" for me when they're all on one line.  I defer
to your greater experience on this matter, however, so just ignore
this patch.


More information about the darcs-users mailing list