Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jun 9, 2013 at 6:22 PM, Célestin Matte
<celestin.matte@ensimag.fr> wrote:
> - strings which don't need interpolation are single-quoted for more clarity and
> slight gain of performance
> - interpolation is preferred over concatenation in many cases, for more clarity
> - variables are always used with the ${} operator inside strings
> - strings including double-quotes are written with qq() so that the quotes do
> not have to be escaped
> ---
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index a66cef4..efc376a 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -200,10 +200,10 @@ sub mw_connect_maybe {
>                                lgdomain => $wiki_domain};
>                 if ($mediawiki->login($request)) {
>                         Git::credential \%credential, 'approve';
> -                       print STDERR "Logged in mediawiki user \"$credential{username}\".\n";
> +                       print STDERR qq(Logged in mediawiki user "$credential{username}".\n);

Given this patch's intention to use ${} within strings, should this be
${credential{username}}?

(I don't have a preference, but it's a genuine question since it's not
clear if this was an oversight or intentional.)

>                 } else {
> -                       print STDERR "Failed to log in mediawiki user \"$credential{username}\" on $url\n";
> -                       print STDERR "  (error " .
> +                       print STDERR qq(Failed to log in mediawiki user "$credential{username}" on ${url}\n);

Ditto: ${credential{username}}

> +                       print STDERR '  (error ' .
>                                 $mediawiki->{error}->{code} . ': ' .
>                                 $mediawiki->{error}->{details} . ")\n";
>                         Git::credential \%credential, 'reject';
> @@ -347,10 +347,10 @@ sub get_mw_pages {
>  #        $out = run_git("command args", "raw"); # don't interpret output as UTF-8.
>  sub run_git {
>         my $args = shift;
> -       my $encoding = (shift || "encoding(UTF-8)");
> -       open(my $git, "-|:$encoding", "git " . $args)
> -           or die "Unable to open: $!\n";
> -       my $res = do {
> +       my $encoding = (shift || 'encoding(UTF-8)');
> +       open(my $git, "-|:${encoding}", "git ${args}")
> +           or die "Unable to fork: $!\n";
> +       my $res = do {

The whitespace-only change to line "my $res = do {" is effectively
noise. The reviewer has to stop and puzzle out what changed on the
line before continuing with review of the remaining _real_ changes. It
is a good idea to avoid noise changes if possible.

In this particular case, it's easy to avoid the noise since the
trailing space on that line could/should have been removed in patch
18/28 when the statement was split over multiple lines.

>                 local $/ = undef;
>                 <$git>
>         };
> @@ -475,26 +475,26 @@ sub download_mw_mediafile {
>                 return $response->decoded_content;
>         } else {
>                 print STDERR "Error downloading mediafile from :\n";
> -               print STDERR "URL: $download_url\n";
> -               print STDERR "Server response: " . $response->code . " " . $response->message . "\n";
> +               print STDERR "URL: ${download_url}\n";
> +               print STDERR 'Server response: ' . $response->code . q{ } . $response->message . "\n";

To meet the goals of this patch, would you want to do this instead?

    "Server response: @{[$response->code]} @{[$response->message]}\n";

Whether this is easier or more difficult to read is a matter of
opinion. (Again, this is a genuine question rather than a show of
preference on my part.)

>                 exit 1;
>         }
>  }
> @@ -691,8 +691,7 @@ sub fetch_mw_revisions {
>         my $n = 1;
>         foreach my $page (@pages) {
>                 my $id = $page->{pageid};
> -
> -               print STDERR "page $n/", scalar(@pages), ": ". $page->{title} ."\n";
> +               print STDERR "page ${n}/", scalar(@pages), ': ', $page->{title}, "\n";

Similarly:

  "page ${n}/@{[scalar(@pages)]}: @{[$page->{title}]}\n"

>                 $n++;
>                 my @page_revs = fetch_mw_revisions_for_page($page, $id, $fetch_from);
>                 @revisions = (@page_revs, @revisions);
> @@ -706,7 +705,7 @@ sub fe_escape_path {
>         }
> -       print STDOUT "N inline :$n\n";
> -       literal_data("mediawiki_revision: " . $commit{mw_revision});
> +       print STDOUT "N inline :${n}\n";
> +       literal_data("mediawiki_revision: $commit{mw_revision}");

As questioned earlier, do you want ${commit{mw_revision}}?

>         print STDOUT "\n\n";
>         return;
>  }
> @@ -911,8 +910,8 @@ sub mw_import_revids {
>                 my $page_title = $result_page->{title};
>
>                 if (!exists($pages->{$page_title})) {
> -                       print STDERR "$n/", scalar(@$revision_ids),
> -                               ": Skipping revision #$rev->{revid} of $page_title\n";
> +                       print STDERR "${n}/", scalar(@$revision_ids),
> +                               ": Skipping revision #$rev->{revid} of ${page_title}\n";

Same question as above regarding formatting via "@{[...]}".

>                         next;
>                 }
>
> @@ -937,14 +936,14 @@ sub mw_import_revids {
>                 # If this is a revision of the media page for new version
>                 # of a file do one common commit for both file and media page.
>                 # Else do commit only for that page.
> -               print STDERR "$n/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";
> +               print STDERR "${n}/", scalar(@$revision_ids), ": Revision #$rev->{revid} of $commit{title}\n";

Ditto.

>                 import_file_revision(\%commit, ($fetch_from == 1), $n_actual, \%mediafile);
>         }