Re: [PATCH 5/7] remote-mediawiki: support fetching from (Main) namespace

From: Antoine Beaupré <anarcat@debian.org>
Date: Nov 2, 2017, 21:19 UTC
Message-ID: <8760as5qfl.fsf@curie.anarc.at>
In-Reply-To: <CAPig+cTX1kBCk-phodTanU1dmwjM_2TNevKyGvdCWonqhEU5Dg@mail.gmail.com>

On 2017-11-01 15:56:51, Eric Sunshine wrote:

Show 18 quoted lines

>> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
>> @@ -264,9 +264,14 @@ sub get_mw_tracked_categories {
>>  sub get_mw_tracked_namespaces {
>>      my $pages = shift;
>>      foreach my $local_namespace (@tracked_namespaces) {
>> -        my $namespace_id = get_mw_namespace_id($local_namespace);
>> +        my ($namespace_id, $mw_pages);
>> +        if ($local_namespace eq "(Main)") {
>> +            $namespace_id = 0;
>> +        } else {
>> +            $namespace_id = get_mw_namespace_id($local_namespace);
>> +        }
>
> I meant to ask this in the previous round, but with the earlier patch
> mixing several distinct changes into one, I plumb forgot: Would it
> make sense to move this "(Main)" special case into
> get_mw_namespace_id() itself? After all, that function is all about
> determining an ID associated with a name, and "(Main)" is a name.

Right. At first sight, I agree: get_mw_namespace_id should do the right thing. But then, I look at the code of that function, and it strikes me as ... well... really hard to actually do this the right way.

In fact, I suspect that passing "" to get_mw_namespace_id would actually do the right thing. The problem, as I explained before, is that passing that in the configuration is pretty hard: it would needlessly complicate the configuration setting, so I think it's a fair shortcut to do it here.

Show 7 quoted lines

>>          next if $namespace_id < 0; # virtual namespaces don't support allpages
>> -        my $mw_pages = $mediawiki->list( {
>> +        $mw_pages = $mediawiki->list( {
>
> Why did the "my" of $my_pages get moved up to the top of the foreach
> loop? I can't seem to see any reason for it. Is this an unrelated
> change accidentally included in this patch?

Just a habit of declaring functions at the beginning of a block. Maybe it's because I'm old? :)

I'll reroll a last patchset with those fixes.

A.

-- 
One of the strongest motives that leads men to art and science is
escape from everyday life with its painful crudity and hopeless
dreariness. Such men make this cosmos and its construction the pivot
of their emotional life, in order to find the peace and security which
they cannot find in the narrow whirlpool of personal experience.
                       - Albert Einstein