On Sun, Jun 9, 2013 at 6:22 PM, Célestin Matte
<celestin.matte@xxxxxxxxxx> 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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html