From: Jakub Narebski <jnareb@gmail.com>
Date: Mon, 27 Sep 2010 08:12:22 GMT
Subject: Re: [PATCHv5 08/12] gitweb: auxiliary function to group data
Message-ID: <201009271012.23175.jnareb@gmail.com>
In-Reply-To: <AANLkTimOJ7RXDWXy=tF+rZf1gnfB7_GHCZuU5bZ5Wc91@mail.gmail.com>

Dnia poniedziałek 27. września 2010 09:26, Giuseppe Bilotta napisał:
> 2010/9/26 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>>
>>> Subject: gitweb: auxiliary function to group data
>>>
>>
>> Errr... what!?  git_group() is not "auxiliary function to group data",
>> but a template for output of group of data.
> 
> I will rephrase

Though I don't know how such rephrasing should look like.

>> It would be probably good to describe how this output looks like
>> (using e.g. ASCII-art mockup) in a commit message.
> 
> Well, that would depend on the CSS that is used ... should I squash
> the styling in this patch then?

No, I don't think it is needed, but it can be done.

>>> +sub git_group {
>>
>> Name?
> 
> git_collection? git_collect_data? I'm a little short on ideas.
> git_section? git_subsection? the function (with different styling) can
> probably be used even for the main sections in each view (think
> summary view in particular).

git_group_html / git_subgroup_html / git_subsection_html
print_group / print_subgroup / print_section / print_subsection

It needs either *_html suffix (like git_header_html), or print_* prefix
to denote that it prints HTML fragment, I think.

>>> +     $content_func->() if defined $content_func;
>>
>> More defensive programming would be to use
>>
>>  +     $content_func->() if ref($content_func) eq 'CODE';
>>
>> Or even:
>>
>>  +     if (ref($content) eq 'CODE') {
>>  +             $content->();
>>  +     } elsif (ref($content) eq 'ARRAY') {
>>  +             print @$content;

The 'ARRAY' part is probably unnecessary overengineering.

>>  +     } elsif (!ref($content) && defined($content)) {
>>  +             print $content;
>>  +     }

Or even (in the vein of further overengineering)

    +     } elsif (ref($content) eq 'SCALAR') {
    +             print esc_html($$content);
    +     } elsif (!ref($content) && defined($content)) {
    +             print $content;
    +     }

or vice versa ;-)

>>
>> Well, $content could be also open filehandle...

Though I don't know how to check that.  ref on filehandles return
'GLOB'... well, we can use 'openhandle' from Scalar::Util (core).
But that is probably unnecessary overengineering.
 
> Ah, very interesting and very flexible, I'll steal your idea.
> 
> -- 
> Giuseppe "Oblomov" Bilotta
> 

-- 
Jakub Narebski
Poland