Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jun 10, 2013, 00:50 UTC
- Message-ID
- <CAPig+cQWQ842-ZG3_JQxZ=-2qLPpijx9z3vtXns+G3xxp6PsyQ@mail.gmail.com>
- In-Reply-To
- <1370816573-3808-23-git-send-email-celestin.matte@ensimag.fr>
On Sun, Jun 9, 2013 at 6:22 PM, Célestin Matte <celestin.matte@ensimag.fr> wrote:
Show 17 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
> ---
> 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}}Show 16 quoted lines
> + 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.
Show 11 quoted 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.)
Show 10 quoted lines
> 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"Show 9 quoted lines
> $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}}?Show 11 quoted lines
> 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 "@{[...]}".Show 9 quoted lines
> 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); > }