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

2013-06-07 Thread Matthieu Moy 
Célestin Matte celestin.ma...@ensimag.fr writes:

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

It would be better to prefix commit messages with git-remote-mediawiki: .

 Fix warnings from perlcritic's level 5 and 4.

It would be cool to have a make perlcritic target in the Makefile so
that future developers can easily re-run it and avoid repeating the same
mistakes. As much as possible, make perlcritic should produce no
output at the end of your patch series (either the warnings should be
fixed, or they should be disabled).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-07 Thread Célestin Matte 
Le 07/06/2013 10:10, Matthieu Moy a écrit :
 It would be cool to have a make perlcritic target in the Makefile so
 that future developers can easily re-run it and avoid repeating the same
 mistakes. As much as possible, make perlcritic should produce no
 output at the end of your patch series (either the warnings should be
 fixed, or they should be disabled).

The problem is that I took some policies into account for some parts of
the code, but not for all of it. For instance, in commit [15/18], I put
some numeric values in constants, but not all of them, as I think having
arg[3] in the code does make sense. Ignoring this policy for future
developement just to prevent the related warnings from appearing would
prevent us to see useful warning messages from this policy.
Therefore, there still are a dozen warning messages appearing in the
current state...

Anyway, should this be integrated in the current patch or added as an
independant patch?

-- 
Célestin Matte
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-07 Thread Matthieu Moy 
Célestin Matte celestin.ma...@ensimag.fr writes:

 The problem is that I took some policies into account for some parts of
 the code, but not for all of it. For instance, in commit [15/18], I put
 some numeric values in constants, but not all of them, as I think having
 arg[3] in the code does make sense. Ignoring this policy for future
 developement just to prevent the related warnings from appearing would
 prevent us to see useful warning messages from this policy.

OK. Perhaps a make sloppy-perlcritic with zero output and a make
pedantic-perlcritic with more policies can make sense. Or just make
perlcritic, and accept that there are warnings.

 Anyway, should this be integrated in the current patch or added as an
 independant patch?

That could be a new patch, preferably the first of the series.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-06 Thread Célestin Matte 
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.

Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@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);
 
 use URI::Escape;
 use IPC::Open2;
-
-use warnings;
+use Readonly;
 
 # Mediawiki filenames can contain forward slashes. This variable decides by 
which pattern they should be replaced
-use constant SLASH_REPLACEMENT = %2F;
+Readonly my $SLASH_REPLACEMENT = %2F;
 
 # It's not always possible to delete pages (may require some
 # privileges). Deleted pages are replaced with this content.
-use constant DELETED_CONTENT = [[Category:Deleted]]\n;
+Readonly my $DELETED_CONTENT = [[Category:Deleted]]\n;
 
 # It's not possible to create empty pages. New empty files in Git are
 # sent with this content instead.
-use constant EMPTY_CONTENT = !-- empty page --\n;
+Readonly my $EMPTY_CONTENT = !-- empty page --\n;
 
 # used to reflect file creation or deletion in diff.
-use constant NULL_SHA1 = ;
+Readonly my $NULL_SHA1 = ;
 
 # Used on Git's side to reflect empty edit messages on the wiki
-use constant EMPTY_MESSAGE = '*Empty MediaWiki Message*';
+Readonly my $EMPTY_MESSAGE = '*Empty MediaWiki Message*';
 
 if (@ARGV != 2) {
exit_error_usage();
@@ -96,6 +96,9 @@ unless ($fetch_strategy) {
$fetch_strategy = by_page;
 }
 
+# Remember the timestamp corresponding to a revision id.
+my %basetimestamps;
+
 # Dumb push: don't update notes and mediawiki ref to reflect the last push.
 #
 # Configurable with mediawiki.dumbPush, or per-remote with
@@ -198,12 +201,14 @@ sub mw_connect_maybe {
exit 1;
}
}
+   return;
 }
 
 ## Functions for listing pages on the remote wiki
 sub get_mw_tracked_pages {
my $pages = shift;
get_mw_page_list(\@tracked_pages, $pages);
+   return;
 }
 
 sub get_mw_page_list {
@@ -219,6 +224,7 @@ sub get_mw_page_list {
get_mw_first_pages(\@slice, $pages);
@some_pages = @some_pages[51..$#some_pages];
}
+   return;
 }
 
 sub get_mw_tracked_categories {
@@ -241,6 +247,7 @@ sub get_mw_tracked_categories {
$pages-{$page-{title}} = $page;
}
}
+   return;
 }
 
 sub get_mw_all_pages {
@@ -260,6 +267,7 @@ sub get_mw_all_pages {
foreach my $page (@{$mw_pages}) {
$pages-{$page-{title}} = $page;
}
+   return;
 }
 
 # queries the wiki for a set of pages. Meant to be used within a loop
@@ -290,6 +298,7 @@ sub get_mw_first_pages {
$pages-{$page-{title}} = $page;
}
}
+   return;
 }
 
 # Get the list of pages to be fetched according to configuration.
@@ -358,11 +367,12 @@ sub get_all_mediafiles {
foreach my $page (@{$mw_pages}) {
$pages-{$page-{title}} = $page;
}
+   return;
 }
 
 sub get_linked_mediafiles {
my $pages = shift;
-   my @titles = map $_-{title}, values(%{$pages});
+   my @titles = map { $_-{title} } values(%{$pages});
 
# The query is split in small batches because of the MW API limit of
# the number of links to be returned (500 links max).
@@ -390,11 +400,13 @@ sub get_linked_mediafiles {
while (my ($id, $page) = each(%{$result-{query}-{pages}})) {
my @media_titles;
if (defined($page-{links})) {
-   my @link_titles = map $_-{title}, 
@{$page-{links}};
+   my @link_titles
+   = map { $_-{title} } @{$page-{links}};
push(@media_titles, @link_titles);
}
if (defined($page-{images})) {
-   my @image_titles = map $_-{title}, 
@{$page-{images}};
+   my @image_titles
+   = map { $_-{title} } @{$page-{images}};
push(@media_titles, @image_titles);

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

2013-06-06 Thread Eric Sunshine 
On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
celestin.ma...@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.ma...@ensimag.fr
 Signed-off-by: Matthieu Moy matthieu@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;
 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html