Re: [PATCH 5/7] gitweb: auxiliary functions to group data

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 17, 2010, 01:24 UTC
Message-ID: <201009170324.25801.jnareb@gmail.com>
In-Reply-To: <1284629465-14798-6-git-send-email-giuseppe.bilotta@gmail.com>

On Thu, 16 Sep 2010, Giuseppe Bilotta wrote:

Show 16 quoted lines

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 6138c6e..92551e4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3765,6 +3765,21 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +sub git_begin_group {
> +	my ($class, $id, @rest) = @_;
> +
> +	$class = ' class="' . join(' ', 'group', $class) . '"';
> +
> +	$id = ' id="' . $id . '"' if $id;
> +
> +	print "<div$class$id>\n";

I agree with Ævar that it would be better to use HTML generation subroutines from CGI.pm, even start_div and end_div...

Show 6 quoted lines

> +	git_print_header_div(@rest);
> +}
> +
> +sub git_end_group {
> +	print "</div>\n"
> +}

... but I think that having separate subroutines for opening and closing tags is a bad design / bad API (except in some rare cases). It is begging for unbalanced HTML.

It would be better if it was a single subroutine wrapping 'div' around contents given either as a string, or via callback (subroutine reference), in my opinion.

Show 8 quoted lines

> +
>  sub print_local_time {
>  	print format_local_time(@_);
>  }
> -- 
> 1.7.3.rc1.230.g8b572
> 
>

-- 
Jakub Narebski
Poland