From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Sat, 18 Sep 2010 07:51:13 GMT
Subject: Re: [PATCH 5/7] gitweb: auxiliary functions to group data
Message-ID: <AANLkTimN+rGCS7pokwSDVT75_LsU4aAsaHmw7Xi_HVm+@mail.gmail.com>
In-Reply-To: <201009171917.44979.jnareb@gmail.com>

On Fri, Sep 17, 2010 at 7:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:
>> On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> Giuseppe Bilotta wrote:
>>>> On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>>>
>>>>> ... 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.
>>>>
>>>> I'm not sure that in this case the string or callback approach would
>>>> be any cleaner. I'll see if perl supports closures or something like
>>>> that.
>>>
>>> Perl supports closures (thanks to anonymous subroutines 'sub { ... }'
>>> and lexical variables 'my $var'), see perlsub and "Function Templates"
>>> in perlref.
>>>
>>> I also recommend free ebook "Higher-Order Perl" http://hop.perl.plover.com/
>>
>> Thanks for the suggestion. I'm still not convinced that such an
>> implementation would be better though. Aside from the general
>> aesthetical suckiness of passing closures around (and the experience
>> is not any more pleasurable in Perl), there's also the matter of the
>> calling convention to use. I can think of two options:
>>
>> (1) we make the function callable as git_do_group($class, $id, sub {
>> <closure that prints the content> }, <paramters that go to
>> git_print_header_div>), which is somewhat illogical since we're
>> specifying the content before the header,
>
> Why not
>
>  git_do_group($class, $id, <print_header_div parameters>, sub { ... })
>
> or even use subroutine prototypes?  We can use 'pop @_' to get last
> argument of a subroutine.

Ah, I hadn't thought about pop to get the last parameter. I think it
makes sense. Ok, I'll give this syntax a try,

> [...]
>> Overall, I still get the impression that the current API is
>> considerably cleaner, even with the small counterweight of the risk of
>> leaving groups open (which is not something so dramatic anyway, IMO).
>
> Might be.
>
> But as currently git_*group() is used in only one place, isn't it
> premature generalization ;-) ?

I noticed the same comment to patch 7/7, still writing the reply for
that patch. I believe the feature is useful enough to deem the
refactoring, and since it's written already why go back? ;-) (it also
makes the code cleaner IMO).


-- 
Giuseppe "Oblomov" Bilotta