Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> Fix warnings from perlcritic's level 5 and 4. They correspond to the following
> cases:
> - always end a submodule with a return
> - don't use the constant pragma, use the Readonly module instead
> - some syntax details for maps, and others.

Although loosely related by being mentioned by perlcritic (4,5), each
bullet point is otherwise unrelated, and mixing such unrelated changes
into a single patch can make review more difficult.

> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |   81 +++++++++++++++++----------
>  1 file changed, 51 insertions(+), 30 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 410eae9..83cf292 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -15,32 +15,32 @@ use strict;
>  use MediaWiki::API;
>  use Git;
>  use DateTime::Format::ISO8601;
> +use warnings;
>
>  # By default, use UTF-8 to communicate with Git and the user
> -binmode STDERR, ":utf8";
> -binmode STDOUT, ":utf8";
> +binmode STDERR, ":encoding(UTF-8)";
> +binmode STDOUT, ":encoding(UTF-8)";

This change isn't explained or rationalized in the commit message.

> @@ -96,6 +96,9 @@ unless ($fetch_strategy) {
>         $fetch_strategy = "by_page";
>  }
>
> +# Remember the timestamp corresponding to a revision id.
> +my %basetimestamps;

Although this is a simple textual relocation, it's not clear why it's
needed or preferable, and the commit message does not explain it.

> @@ -473,9 +486,6 @@ sub get_last_local_revision {
>         return $lastrevision_number;
>  }
>
> -# Remember the timestamp corresponding to a revision id.
> -my %basetimestamps;
> -
>  # Get the last remote revision without taking in account which pages are
>  # tracked or not. This function makes a single request to the wiki thus
>  # avoid a loop onto all tracked pages. This is useful for the fetch-by-rev
> @@ -555,7 +565,7 @@ sub mediawiki_smudge {
>
>  sub mediawiki_clean_filename {
>         my $filename = shift;
> -       $filename =~ s/@{[SLASH_REPLACEMENT]}/\//g;
> +       $filename =~ s{$SLASH_REPLACEMENT}{/}g;

Although patch 2/18 replaces regex // with {}, the change sneaked into
this patch (1/18) prematurely.

>         # [, ], |, {, and } are forbidden by MediaWiki, even URL-encoded.
>         # Do a variant of URL-encoding, i.e. looks like URL-encoding,
>         # but with _ added to prevent MediaWiki from thinking this is
> @@ -569,7 +579,7 @@ sub mediawiki_clean_filename {
>
>  sub mediawiki_smudge_filename {
>         my $filename = shift;
> -       $filename =~ s/\//@{[SLASH_REPLACEMENT]}/g;
> +       $filename =~ s{/}{$SLASH_REPLACEMENT}g;

Ditto regarding // to {}.

>         $filename =~ s/ /_/g;
>         # Decode forbidden characters encoded in mediawiki_clean_filename
>         $filename =~ s/_%_([0-9a-fA-F][0-9a-fA-F])/sprintf("%c", hex($1))/ge;
> @@ -588,7 +599,8 @@ sub literal_data_raw {
>         utf8::downgrade($content);
>         binmode STDOUT, ":raw";
>         print STDOUT "data ", bytes::length($content), "\n", $content;
> -       binmode STDOUT, ":utf8";
> +       binmode STDOUT, ":encoding(UTF-8)";

Unexplained change.

> +       return;
> }
>
>  sub mw_capabilities {
> @@ -1314,7 +1334,8 @@ sub get_mw_namespace_id {
>  }
>
>  sub get_mw_namespace_id_for_page {
> -       if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
> +       my $namespace = shift;
> +       if ($namespace =~ /^([^:]*):/) {

Another change not mentioned by the commit message.

>                 return get_mw_namespace_id($namespace);
>         } else {
>                 return;
> --