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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 8, 2013, 00:39 UTC
Message-ID: <CAPig+cT6Qw1w4x95oT7PctMz_HnpU0Vwv5Bh=n-uqZQWJjFnBw@mail.gmail.com>
In-Reply-To: <1370641344-4253-17-git-send-email-celestin.matte@ensimag.fr>

On Fri, Jun 7, 2013 at 5:42 PM, Célestin Matte <celestin.matte@ensimag.fr> wrote:

Show 6 quoted lines

> - 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

Distinct changes could (IMHO) be split into separate patches for easier review.

Show 6 quoted lines

> Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>
> Conflicts:
>
>         contrib/mw-to-git/git-remote-mediawiki.perl

Conflict information is not interesting to reviewers or even in final commit messages since (hopefully) you will have resolved conflicts before committing. They can be stripped.

More below.

Show 19 quoted lines

> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |  244 +++++++++++++--------------
>  1 file changed, 121 insertions(+), 123 deletions(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl
> index f37488b..2d4ea1d 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -199,10 +199,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);
>                 } 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);
> +                       print {*STDERR} '  (error ' .

This change from patch 17/22 (adding braces around file handles) sneaked in early with this patch (16/22).

Show 13 quoted lines

>                                 $mediawiki->{error}->{code} . ': ' .
>                                 $mediawiki->{error}->{details} . ")\n";
>                         Git::credential \%credential, 'reject';
> @@ -473,27 +473,27 @@ sub download_mw_mediafile {
>         if ($response->code == 200) {
>                 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} "Error downloading mediafile from :\n";
> +               print {*STDERR} "URL: ${download_url}\n";
> +               print {*STDERR} 'Server response: ' . $response->code . q{ } . $response->message . "\n";

Ditto: Sneak in from 17/22.

Show 32 quoted lines

>                 exit 1;
>         }
>  }
>
>  sub get_last_local_revision {
>         # Get note regarding last mediawiki revision
> -       my $note = run_git("notes --ref=$remotename/mediawiki show refs/mediawiki/$remotename/master 2>/dev/null");
> +       my $note = run_git("notes --ref=${remotename}/mediawiki show refs/mediawiki/${remotename}/master 2>/dev/null");
>         my @note_info = split(/ /, $note);
>
>         my $lastrevision_number;
> -       if (!(defined($note_info[0]) && $note_info[0] eq "mediawiki_revision:")) {
> -               print STDERR "No previous mediawiki revision found";
> +       if (!(defined($note_info[0]) && $note_info[0] eq 'mediawiki_revision:')) {
> +               print STDERR 'No previous mediawiki revision found';
>                 $lastrevision_number = 0;
>         } else {
>                 # Notes are formatted : mediawiki_revision: #number
>                 $lastrevision_number = $note_info[1];
>                 chomp($lastrevision_number);
> -               print STDERR "Last local mediawiki revision found is $lastrevision_number";
> +               print STDERR "Last local mediawiki revision found is ${lastrevision_number}";
>         }
>         return $lastrevision_number;
>  }
> @@ -690,8 +690,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";

Again 17/22.

Show 12 quoted lines

>                 $n++;
>                 my @page_revs = fetch_mw_revisions_for_page($page, $id, $fetch_from);
>                 @revisions = (@page_revs, @revisions);
> @@ -705,7 +704,7 @@ sub fe_escape_path {
>      $path =~ s/\\/\\\\/g;
>      $path =~ s/"/\\"/g;
>      $path =~ s/\n/\\n/g;
> -    return '"' . $path . '"';
> +    return qq("${path}");
>  }
>
>  sub import_file_revision {