Re: [PATCH v4 2/2] gitweb: introduce localtime feature

From: Kevin Cernekee <cernekee@gmail.com>
Date: Mar 19, 2011, 19:49 UTC
Message-ID: <AANLkTimssscn+STEPyM7NbXF5ddFApPBsgXfqz-9SSNs@mail.gmail.com>
In-Reply-To: <7v39mjro38.fsf@alter.siamese.dyndns.org>

On Sat, Mar 19, 2011 at 10:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> Looks like we used to only paint HH:MM part but...
> ... we now paint the whole line, which I personally think is a friendly
> move for color challenged people (me included---a larger span of text
> painted in different colors tends to help you still notice it better using
> value/brightness difference, even if your hue perception is weaker than
> other people). But it is a change from the old behaviour and might be
> worth stating in the log message.

For the $feature{'localtime'} disabled case, the coloring is the same as before.

I will paint the whole line in the next spin, and mention it in the commit message.

Show 7 quoted lines

>
>> -     return $localtime;
>> +     if ($use_localtime) {
>> +             $timestamp .= $alt_time;
>> +     }
>
> You kept -localtime nobody uses?

Show 10 quoted lines

> While I don't agree with Jakub about the presense / absence of "[]" around
> it (that is not a part of "timestamp", but is how the caller wants to
> prepare the space to plug a timestamp string in), I do agree that the
> second parameter "use-localtime" looks funny, not because it is an unnamed
> parameter (which I personally think is fine) but because it isn't about
> the localness of the displayed time anymore.  Your 'localtime' feature
> alone controls in which timezone the timestamp is shown, and this only
> controls the use of 'atnight' highlighting. The parameter needs to be
> renamed, and perhaps you may clarify it further by making it a keyword
> argument as Jakub suggests.

$use_localtime indicates whether or not to add the " (hh:mm -zzzz)" at the end. This also enables the atnight coloring.

This argument name was suggested in an earlier post and I guess I took it a little too literally...

Do you think it would be a good idea to take two separate options: -atnight for the variable coloring, and -alt_time (or some other name) to show " (hh:mm -zzzz)" after the RFC 2822 string?

Or maybe take one option, named something like "-commitpage", to indicate that it is a format specific to that view? If it is not specified, the caller gets back an uncolored RFC 2822 date.

Also, is there a cleaner way of writing this?

sub timestamp_html {
    my %date = %{$_[0]};
    shift;
    my %opts = @_;

Or should I pass in the options as a hash reference, more like $cgi->a():

sub timestamp_html {
    my %date = %{$_[0]};
    my %opts = %{$_[1]};

Thanks.