Re: [PATCH 1/2] gitweb: rename parse_date() to format_date()
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 19, 2011, 10:33 UTC
- Message-ID
- <201103191134.00098.jnareb@gmail.com>
- In-Reply-To
- <ab54ba2199cc7487e383a31e3aa65885@localhost>
On Sat, 19 Mar 2011, Kevin Cernekee wrote:
> One might reasonably expect a function named parse_date() to be used
> for something along these lines:
>
> $unix_time_t = parse_date("2011-03-19");First, one would expect parse_date() to return either list of year, month, etc., e.g. [2011, 3, 19, ...], or a hash with named date fragments, e.g. {'year' => 2011, 'month' => 3, ...}.
Almost all other parse_* subroutines in gitweb (parse_commit, parse_tag, parse_difftree_raw_line, parse_ls_tree_line, with the sole exception of parse_from_to_diffinfo with perhaps a bit misleading name) return hash of parsed and pre-formatted fragments.
So if parse_date(1300505805) returned
(
'hour' => 3,
'minute' => 36,
...
)_without_ the 'rfc2822', 'mday-time' (unused), 'iso-8601', 'iso-tz', it would be not misnamed parse_*.
Second, it is _date_ because it parses / processes dates in git internal format. Perhaps it should be called *_raw_date, or *_git_date, or *_epoch, or *_timestamp and not *_date.
> But instead, gitweb's parse_date works more like:
>
> &parse_date(1300505805) = {Note: parse_date takes second argument, which is numeric timezone (defaults to '-0000'), so one usually uses
parse_date(1300505805, '+0200')
Show 6 quoted lines
> 'hour' => 3, > 'minute' => 36, > ... > 'rfc2822' => 'Sat, 19 Mar 2011 03:36:45 +0000', > ... > }
What I would like to see is to split parsing date from generating formatted dates into e.g. parse_date and formatted_date, and replace calls to parse_date with calls to formatted_date (which uses parse_date internally).
> Rename the function [to format_date] to improve clarity. > No change to functionality.
But name *format_date* is _not_ a good name. All other format_* subroutines in gitweb return _formatted string_, not a hash which contains many formatted strings.
It is hard to come up with really good name, but perhaps one from the ;ist below would be good idea:
- formatted_date - process_date, process_raw_date, process_git_date - process_epoch, process_timestamp - date_formats - from_epoch, date_from_epoch
Any other ideas?
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com> > --- > gitweb/gitweb.perl | 18 +++++++++--------- > 1 files changed, 9 insertions(+), 9 deletions(-)
Show 9 quoted lines
> @@ -4906,7 +4906,7 @@ sub git_log_body {
> next if !%co;
> my $commit = $co{'id'};
> my $ref = format_ref_marker($refs, $commit);
> - my %ad = parse_date($co{'author_epoch'});
> + my %ad = format_date($co{'author_epoch'});
> git_print_header_div('commit',
> "<span class=\"age\">$co{'age_string'}</span>" .
> esc_html($co{'title'}) . $ref,Hmm, we should probably fix it so it reads
- my %ad = parse_date($co{'author_epoch'});
+ my %ad = formatted_date($co{'author_epoch'}, $co{'author_tz'});Especially when preparing for 'localtime' feature.
Show 9 quoted lines
> @@ -7064,7 +7064,7 @@ sub git_feed {
> if (defined($commitlist[0])) {
> %latest_commit = %{$commitlist[0]};
> my $latest_epoch = $latest_commit{'committer_epoch'};
> - %latest_date = parse_date($latest_epoch);
> + %latest_date = format_date($latest_epoch);
> my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
> if (defined $if_modified) {
> my $since;Similarly here.
Show 9 quoted lines
> @@ -7195,7 +7195,7 @@ XML
> if (($i >= 20) && ((time - $co{'author_epoch'}) > 48*60*60)) {
> last;
> }
> - my %cd = parse_date($co{'author_epoch'});
> + my %cd = format_date($co{'author_epoch'});
>
> # get list of changed files
> open my $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,Same here.
-- Jakub Narebski Poland