Jeff King [off-list ref] writes:
Something like this (instead of my last patch):
Yeah, I like that much better, especially the renaming of
$use_color to more descriptive (but is it really about "menu", I
wonder?).
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 = ...;
}
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.
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
quoted
+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