Re: [PATCH] gitweb: allow extra breadcrumbs to prefix the trail

From: Jakub Narębski <jnareb@gmail.com>
Date: Jul 4, 2013, 16:32 UTC
Message-ID: <CANQwDwfNrqZvLNYS6kqZdgX-ab5fK3RmQqAN3qKTW2TGnaDaUg@mail.gmail.com>
In-Reply-To: <alpine.LSU.2.00.1307041646250.26246@hermes-2.csi.cam.ac.uk>

On Thu, Jul 4, 2013 at 5:56 PM, Tony Finch <dot@dotat.at> wrote:

Show 7 quoted lines

> Jakub Narębski <jnareb@gmail.com> wrote:
>>
>> First, do I understand correctly that @extra_breadcrumbs are rendered *after*
>> $home_link*, and in exactly the same manner?
>
> Before the home link, and yes, in the same manner. The extra breadcrumbs
> are for links to parent pages above gitweb in some hierarchy.

Hmmm... I would have thought that they were after home link. I wonder if leaving it up to user to configure @extra_breadcrumbs to include $home_link in appropriate place (the unshift / push solution to adding to @extra_breadcrumbs, starting with $home_link) would be good idea, or over-engineering.

In what situation do you need those extra breadcrumbs useful? What necessity / itch to scratch is behind idea of this patch?

Show 10 quoted lines

>> But now I think that we can do better, simply put $home_link_str and $home_link
>> in @extra_breadcrumbs / @top_level_breadcrumbs / @nav_breadcrumbs before
>> using it,
>
> We could save a line that way:
>
> -       print $cgi->a({-href => esc_url($home_link)}, $home_link_str) . " / ";
> +       for my $crumb (@extra_breadcrumbs, [ $home_link_str => $home_link ]) {
> +               print $cgi->a({-href => esc_url($crumb->[1])}, $crumb->[0]) . " / ";
> +       }

And avoid a bit of code duplication; now we are sure that both @extra_breadcrumbs and $home_link are rendered in the same way.

Show 6 quoted lines

>> P.S. It is a bit late, but wouldn't { name => $link_name, href => $link_url }
>> (like %features hash) be a better solution than [ $link_name, $link_url ],
>> i.e. hashref (named parameters) instead of arrayref (positional parameters).
>> You wouldn't have to remember which is first: text or URL.
>
> I thought the fat arrow would be mnemonic enough, and less verbose.

Yes, hashref solution is a bit verbose. I don't like abusing fat arrow notation, but here it gives nice mnemonic (hopefully explained in documentation).

-- 
Jakub Narebski