Re: [PATCH 01/18] Follow perlcritic's recommendations - level 5 and 4
From: Eric Sunshine <hidden>
Date: 2016-06-15 22:57:34
On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte [off-list ref] 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Célestin Matte <redacted> Signed-off-by: Matthieu Moy <redacted> --- 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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
# [, ], |, {, 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 {}.
quoted hunk ↗ jump to hunk
$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.
quoted hunk ↗ jump to hunk
+ 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;
--