On Sun, Jan 06, 2008 at 04:32:15AM -0800, Junio C Hamano wrote:
> Yeah, I like that much better, especially the renaming of
> $use_color to more descriptive (but is it really about "menu", I
> wonder?).
I thought that, too, but I didn't know what better word to use
(something like "display" doesn't make it clear that it doesn't involve
the diff).
> I might consider rewriting this part
>
> > +my $menu_use_color = $repo->get_colorbool('color.interactive');
> > +my ($prompt_color, $header_color, $help_color) =
> > + $menu_use_color ? (
> > + $repo->get_color('color.interactive.prompt', 'bold blue'),
> > + $repo->get_color('color.interactive.header', 'bold'),
> > + $repo->get_color('color.interactive.help', 'red bold'),
> > + ) : ();
>
> like this:
>
> my ($prompt_color, $header_color, $help_color, $fraginfo_color);
> if ($colored_prompt) {
> $prompt_color = ...;
> $header_color = ...;
> }
> if ($colored_diff) {
> $fraginfo_color = ...;
> }
Actually, that's more or less how it's written before my patch (in fact,
you could eliminate that part of my patch and just move $normal_color
outside of the conditional). However I didn't like having the
declaration and the assignment so far apart (and duplicated). But I will
admit my version is a little nested. Please feel free to switch it when
you apply.
> or even like this:
>
> my (%palette);
> if ($colored_prompt) {
> my %default = (
> prompt => 'bold blue',
> header => 'bold',
> ...
> );
> while (my ($k,$v) = each %default) {
> $palette{$k} = $repo->get_color("color.interactive.$k",$v);
> }
> }
>
> But I realize that's going overboard. Certainly the last one is
> doing unnecessary generalization for generalization's sake.
Yes, I considered making a %palette, as well, but it just seemed a
little gratuitous (and the nice thing about using variables is that they
catch typos better).
-Peff
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
