[darcs-devel] darcs patch: Little style change (avoid a bind+return). (and 1 more)

[David Roundy]

On Tue, Apr 15, 2008 at 05:01:48PM +0200, Nicolas Pouillard wrote:
> Excerpts from David Roundy's message of Tue Apr 15 16:44:09 +0200 2008:
> > On Mon, Apr 14, 2008 at 11:25:55PM +0000, Nicolas Pouillard wrote:
> > > Sun Apr 13 16:23:18 CEST 2008  Nicolas Pouillard <nicolas.pouillard at gmail.com>
> > >   * Little style change (avoid a bind+return).
> > 
> > This change is buggy:  if and when type witnesses are supported in this
> > module, using a let binding will cause ghc's brain to explode.
> 
> Oops, really sorry.

No problem.  It's a stupid thing we're forced to do by the language
limitations.  The trouble is just that let bindings can be recursive, and
are generally much harder to apply type inference to than case bindings or
monad pattern matchings (which desugars to case binding).

> > > Mon Apr 14 23:45:59 CEST 2008  nicolas.pouillard at gmail.com
> > >   * Extend a little color printing, and prettify hunks with mangenta and cyan.
> > 
> > Could you break this into smaller patches, please?
> Yes I will do that.
> 
> > This is a rather large
> > change, and I don't really have a clear picture of what all the parts are
> > doing.  It looks like you've mixed together some refactoring with some
> > changes from British to American spelling with some feature additions, and
> > I can't really distinguish which is which (well, I *was* able to identify
> > the spelling change, but I don't see any reason to include it).
> 
> The  spelling  change  was  more  about  normalization since 'color' seems for
> often used than 'colour'. However I will not include it in my next patches.

I don't mind the spelling change, but it should be a separate patch, so
that folks looking at the output of annotate can see why the change was
made, since a pure-spelling patch would have this information in it.

> > Also, I don't see any mechanism to control whether this change in
> > formatting is used, which should really be there.  But since I haven't
> > compiled and run with this change (and can't tell what it does from
> > reading either the code or your description thereof), I don't have a
> > clear idea how garish it actually is.  I suspect that in any case we'll
> > want to allow users (using environment variables) to configure whether
> > the extra coloration is used.
> 
> Ok, I can add one other environment variable and one more policy option.
> 
> However  I don't really see a way to add this control mechanism without either
> adding a special case for magenta and cyan colors; or adding extra parameters
> to printing functions.

I think a special case for magenta and cyan is quite reasonable, as they
are quite different from red/green/blue, and might be illegible for users
with a white background.  A more thorough solution would allow users to
specify custom colors for various items (and bold/unbold), but there's
never been a serious call for that complexity.  Most folks who don't like
the default coloring seem to be happy with no coloring, as far as I can
tell.

Incidentally, the reason blue is the only color used in the default
configuration of darcs (although red is now used for darcs-2 conflicts) is
because it's the only color that I've been happy with in my terminals
whether I'm using white or black backgrounds.  Currently I use only
black-background terminals, but I used to use terminals with a white
background, and colors like green, cyan and magenta were pretty close to
illegible on the terminal I used to use.  Red had sufficient contrast, but
I don't like red.

And to explain my explanation:  as long as we don't give our users a choice
in any particular matter, my preference is the only one that I can go by.
Or perhaps, I should say it's the only preference that I'm willing to go
by.  Once in the past I made a change (I don't remember what) in darcs that
I didn't like, because a few people strongly argued that it would be
better, only to find out that a large majority prefered the previous
behavior...
-- 
David Roundy
Department of Physics
Oregon State University