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

On Fri, Sep 17, 2010 at 7:17 PM, Jakub Narebski [off-list ref] wrote:

On Fri, 17 Sep 2010, Giuseppe Bilotta wrote:

quoted

On Fri, Sep 17, 2010 at 6:06 PM, Jakub Narebski [off-list ref] wrote:

quoted

Giuseppe Bilotta wrote:

quoted

On Fri, Sep 17, 2010 at 3:24 AM, Jakub Narebski [off-list ref] wrote:

quoted

... 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,

[...]

quoted

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